From: Eli Zaretskii <eliz@gnu.org>
To: fabrice nicol <fabrnicol@gmail.com>
Cc: 47408@debbugs.gnu.org
Subject: bug#47408: Etags support for Mercury [v0.3]
Date: Sun, 28 Mar 2021 16:11:51 +0300 [thread overview]
Message-ID: <834kgvo220.fsf@gnu.org> (raw)
In-Reply-To: <5ba2fec3-3f61-fb7e-35eb-7188fa6064a4@gmail.com> (message from fabrice nicol on Sat, 27 Mar 2021 11:51:22 +0100)
> From: fabrice nicol <fabrnicol@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@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".
> \f
> * 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.
next prev parent reply other threads:[~2021-03-28 13:11 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <25b8baef-11f2-7079-69d8-3207a24658fc@gmail.com>
2021-03-26 7:09 ` bug#47408: Emacs etags support for Mercury [v0.2] fabrice nicol
2021-03-27 10:51 ` bug#47408: Etags support for Mercury [v0.3] fabrice nicol
2021-03-28 13:11 ` Eli Zaretskii [this message]
2021-03-28 15:49 ` fabrice nicol
2021-03-28 16:22 ` Eli Zaretskii
2021-03-29 11:53 ` bug#47408: Etags support for Mercury [v0.4] fabrice nicol
[not found] ` <70503251-f8ea-9006-b7e7-b13b93bb71de@gmail.com>
2021-05-15 8:31 ` bug#47408: Fwd: " Eli Zaretskii
[not found] ` <53162dfb-0715-3077-78d1-3a8340943f2f@gmail.com>
2021-05-29 8:01 ` Eli Zaretskii
[not found] ` <CANTSrJtDMu=4SWUcBRt51X8n42mOfB6_sFi8mNoZ0YgYdtE-DA@mail.gmail.com>
2021-05-29 10:22 ` Eli Zaretskii
2021-06-01 2:38 ` bug#47408: Etags support for Mercury [v0.5] fabrice nicol
2021-06-06 9:48 ` Eli Zaretskii
2021-06-06 13:34 ` fabrice nicol
2021-06-06 18:18 ` Francesco Potortì
2021-06-06 20:49 ` fabrice nicol
2021-06-06 21:04 ` Francesco Potortì
2021-06-07 12:13 ` Eli Zaretskii
2021-06-08 0:38 ` Fabrice Nicol
2021-06-08 10:53 ` Francesco Potortì
2021-06-08 11:47 ` Eli Zaretskii
2021-06-08 12:47 ` Francesco Potortì
2021-06-10 13:59 ` Eli Zaretskii
2021-06-10 16:52 ` fabrice nicol
2021-06-10 17:05 ` Francesco Potortì
2021-06-10 17:20 ` Eli Zaretskii
2021-06-10 19:15 ` Eli Zaretskii
2021-06-10 20:39 ` bug#47408: Etags support for Mercury -- fix explicit tags for existentially-quantified procedures fabrice nicol
2021-06-11 5:56 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=834kgvo220.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=47408@debbugs.gnu.org \
--cc=fabrnicol@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).