Package: emacs;
Reported by: fabrice nicol <fabrnicol <at> gmail.com>
Date: Fri, 26 Mar 2021 08:28:02 UTC
Severity: normal
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
Message #13 received at 47408 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: fabrice nicol <fabrnicol <at> gmail.com> Cc: 47408 <at> debbugs.gnu.org Subject: Re: bug#47408: Etags support for Mercury [v0.3] Date: Sun, 28 Mar 2021 16:11:51 +0300
> From: fabrice nicol <fabrnicol <at> gmail.com>
> Date: Sat, 27 Mar 2021 11:51:22 +0100
>
> I'm sending a new patch for this Mercury feature request.
Thanks, I have some comments below.
> >From 50f3f9a0d46d11d0ac096f79f0d5aa1bc17b7920 Mon Sep 17 00:00:00 2001
> From: Fabrice Nicol <fabrnicol <at> gmail.com>
> Date: Sat, 27 Mar 2021 10:16:44 +0100
> Subject: [PATCH] Fixed regressions caused by Objc/Mercury ambiguous file
> extension .m.
Please accompany the changeset with a ChangeLog-style commit log
message, you can see the style we are using via "git log" and also
find some instructions in CONTRIBUTE.
> .B \-V, \-\-version
> Print the current version of the program (same as the version of the
> emacs \fBetags\fP is shipped with).
> +.TP
> +.B \-, \-\-version
> +Print the current version of the program (same as the version of the
> +emacs \fBetags\fP is shipped with).
Copy/paste mistake? or why are you duplicating the --version
description?
> +.TP
> +.B \-M, \-\-no\-defines
> +For the Mercury programming language, tag both declarations and
> +definitions. Declarations start a line with \fI:\-\fP optionally followed by a
> +quantifier over a variable (\fIsome [T]\fP or \fIall [T]\fP), then by
> +a builtin operator like \fIpred\fP or \fIfunc\fP.
> +Definitions are first rules of clauses, as in Prolog.
> +Implies \-\-language=mercury.
> +.TP
> +.B \-m, \-\-declarations
> +For the Mercury programming language, tag declarations as with \fB\-M\fP, but do not
> +tag definitions. Implies \-\-language=mercury.
This is not what Francesco Potortì suggested to do. He suggested that
you use the existing options --no-defines and --declarations, but give
them Mercury-specific meanings when processing Mercury source files.
IOW, let's not introduce the new -m and -M shorthands for these options,
and let's describe the Mercury-specific meaning of the existing
options where they are currently described in etags.1. OK?
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -93,6 +93,15 @@ useful on systems such as FreeBSD which ships only with "etc/termcap".
>
> * Changes in Emacs 28.1
>
> +---
^^^
This should be "+++", since you submitted the changes for the
documentation as part of the changeset.
> +** Etags support for the Mercury programming language (https://mercurylang.org).
> +** New etags command line options '-M/-m' or --declarations/--no-defines'.
> +Tags all Mercury declarations. For compatibility with Prolog etags support,
> +predicates and functions appearing first in clauses will be tagged if etags is
> +run with the option '-M' or '--declarations'. If run with '-m' or
> +'--no-defines', declarations will be tagged but definitions will not.
> +Both options imply --language=mercury.
This should be amended for the changes in the options I described
above.
> +/* Define MERCURY_HEURISTICS_RATIO as it was necessary to disambiguate
> + Mercury from Objective C, which have same file extensions .m */
This comment should explain how the value is used to disambiguate, so
that people could decide what alternative value to use.
> +static void test_objc_is_mercury(char *, language **);
^^
Our style is to leave one space between the function's name and the
opening parenthesis. Please follow that here and elsewhere in your
patch.
> @@ -621,7 +629,6 @@ #define STDIN 0x1001 /* returned by getopt_long on --parse-stdin */
> "In Java code, all the tags constructs of C and C++ code are\n\
> tagged. (Use --help --lang=c --lang=c++ --lang=java for full help.)";
>
> -
> static const char *Cobol_suffixes [] =
> { "COB", "cob", NULL };
> static char Cobol_help [] =
Why remove this empty line?
> static const char *Objc_suffixes [] =
> - { "lm", /* Objective lex file */
> - "m", /* Objective C file */
> - NULL };
> + {"lm",
> + "m", /* By default, Objective C will be assumed. */
> + NULL};
This loses the explanation that a .lm file is an ObjC lex file.
> @@ -773,7 +792,6 @@ #define STDIN 0x1001 /* returned by getopt_long on --parse-stdin */
> 'TEXTAGS' to a colon-separated list like, for example,\n\
> TEXTAGS=\"mycommand:myothercommand\".";
>
> -
> static const char *Texinfo_suffixes [] =
> { "texi", "texinfo", "txi", NULL };
> static const char Texinfo_help [] =
Again, an empty line removed -- why?
> + puts ("-m, --declarations\n\
> + For the Mercury programming language, only tag declarations.\n\
> + Declarations start a line with :- \n\
> + Implies --language=mercury.");
> +
> + puts ("-M, --no-defines\n\
> + For the Mercury programming language, include both declarations and\n\
> + definitions. Declarations start a line with :- while definitions\n\
> + are first rules for a given item, as for Prolog.\n\
> + Implies --language=mercury.");
> +
This should be merged with the existing description of the long
options.
> /* When the optstring begins with a '-' getopt_long does not rearrange the
> non-options arguments to be at the end, but leaves them alone. */
> - optstring = concat ("-ac:Cf:Il:o:Qr:RSVhH",
> + optstring = concat ("-ac:Cf:Il:Mmo:Qr:RSVhHW",
> (CTAGS) ? "BxdtTuvw" : "Di:",
> "");
As mentioned, let's not introduce -m and -M.
> + case 'M':
> + with_mercury_definitions = true; FALLTHROUGH;
> + case 'm':
> + {
> + language lang =
> + { "mercury", Mercury_help, Mercury_functions, Mercury_suffixes };
> +
> + argbuffer[current_arg].lang = ⟨
> + argbuffer[current_arg].arg_type = at_language;
> + }
> + break;
Shouldn't be needed anymore.
> /* Etags options */
> - case 'D': constantypedefs = false; break;
> + case 'D': constantypedefs = false; break;
This whitespace change is for the worse: our conventions are to use
mixed spaces-with-tabs style for indentation in C source files, not
just spaces.
> +static void test_objc_is_mercury(char *this_file, language **lang)
Our style is to write function definitions like this:
static void
test_objc_is_mercury (char *this_file, language **lang)
IOW, break the line between the return type and the function's name.
> + FILE* fp = fopen(this_file, "r");
> + if (fp == NULL) return;
No error/warning if the file couldn't be open?
In any case, this leaks a FILE object: you open a file, but never
close it.
> + uint64_t lines = 1;
> + uint64_t mercury_decls = 0;
We don't use such types elsewhere in etags.c; why do you need them
here? can you use intmax_t instead, as we do elsewhere?
> + case '%': FALLTHROUGH;
> + case ' ': FALLTHROUGH;
> + case '\t':
> + start_of_line = false;
FALLTHROUGH isn't needed here, as there's no code under the first 2
'case' lines.
> + /* Change the language from Objective C to Mercury */
Our style for comments is to end each comment with a period and 2
spaces, like this:
/* Change the language from Objective C to Mercury. */
Please follow this style, here and elsewhere in the changeset.
> + uint8_t decl_type_length = pos - origpos;
Please use 'unsigned char' instead of uint8_t.
> + if (( (s[len] == '.' /* This is a statement dot, not a module dot. */
> + || (s[len] == '(' && (len += 1))
> + || (s[len] == ':' /* Stopping in case of a rule. */
> + && s[len + 1] == '-'
> + && (len += 2)))
> + && (lastlen != len || memcmp (s, last, len) != 0)
> + )
> + /* Types are often declared on several lines so keeping just
> + the first line */
> + || is_mercury_type
> + )
Please avoid parentheses alone on their lines.
> diff --git a/lisp/speedbar.el b/lisp/speedbar.el
> index 12e57b1108..63f3cd6ca1 100644
> --- a/lisp/speedbar.el
> +++ b/lisp/speedbar.el
> @@ -3534,6 +3534,8 @@ speedbar-fetch-etags-parse-list
> speedbar-parse-c-or-c++tag)
> ("^\\.emacs$\\|.\\(el\\|l\\|lsp\\)\\'" .
> "def[^i]+\\s-+\\(\\(\\w\\|[-_]\\)+\\)\\s-*\C-?")
> + ("^\\.m$\\'" .
> + "\\(^:-\\)?\\s-*\\(\\(pred\\|func\\|type\\|instance\\|typeclass\\)+\\s+\\([a-z]+[a-zA-Z0-9_]*\\)+\\)\\s-*(?^?")
> ; ("\\.\\([fF]\\|for\\|FOR\\|77\\|90\\)\\'" .
> ; speedbar-parse-fortran77-tag)
> ("\\.tex\\'" . speedbar-parse-tex-string)
What about ObjC here? or are these keywords good for ObjC as well?
Last, but not least: if you can, please provide a test file for the
etags test suite, see test/manual/etags/.
Thanks again for working on this.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.