bug#47408: Etags support for Mercury [v0.3]

[fabrice nicol <fabrnicol@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6495 bytes --]

Thanks for this review.

Changes will be implemented soon as indicated.

(1) There is just one point that I would like to discuss before changing 
things around: the proposed -m/-M short option issue.


I left this couple of options in (following Francesco Potorti only for 
long options --declarations/--no-defines), for two reasons:

1. The ambiguity between Objective C and Mercury

Both languages having the same file extension .m, it was necessary to 
add in a heuristic test function, in the absence of explicit language 
identification input from command line.

Yet all heuristics may fail in rare cases. Tests show a fairly low 
failure rate on the Mercury compiler source code.  Less than 0.5 % of .m 
files are not identified as Mercury files by the test (this should have 
been documented somewhere).  File concerned by test failure are some 
Mercury test files and documentary test files with only (or almost only) 
comments and blank lines.

While this could be improved by tweaking the heuristic test, it would 
make it more complex, bug-prone and ultimately hard to maintain.

So -m/-M are useful to deal with these rare files, as they do not rely 
on the heuristic test function at all but on their own semantics, which 
explicitly identifies Mercury.

The only alternative I see is to explicitly warn users about adding '-l 
mercury' to command line when using long options (in etags.1 and 
possibly other docs).

Whether this is less intrusive (or more) than -m/-M is not crystal-clear 
to me.  Both solutions look on a par wrt this criterion, but -m/-M may 
be more user-friendly.

If two short options are one too many, I propose redesigning the short 
option pair as just one -m option with a binary argument (like: '-m 
defines / -m all', or -m 0 / -m 1).


2. The social side of things

As indicated previously, I also consulted the Mercury review list, and 
the feedback was positive on -m/-M (see below):

>     Accommodating different people's different preferences is a good idea
>     if it can be done at acceptable cost.
>
>>     Instead of -M, you should use --declarations
>>
>>     Instead of -m, you should use --no-defines
>     There is no need for "instead"; you can support both forms of both options.
>
So I opted for a compromise: renaming long options, following F. 
Potorti, and keeping -m/-M, following Z. Somogyi.


(2) Your following question:


>     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?

has the following reply: Objective C .m files are not parsed by 
speedbar.el in current repository code, so the added feature does not 
break anything.  Issues will only arise if/when Emacs maintainers for 
Objective C support decide on adding this file format to the speedbar 
parser.   It would be premature (and out-of-place) for me to settle this 
on my own.  Should this move happen, the heuristics used in etags.c 
(function test_objc_is_mercury) could then be ported to elisp code.


>> +.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?

> +** 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.
> 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.
>
>> 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.

[-- Attachment #2: Type: text/html, Size: 9323 bytes --]