[PATCH] Provide diagnostic hints for missing inttypes.h string constants.

[Mark Wielaard]

Hi,

On Fri, May 22, 2020 at 09:42:28AM -0400, David Malcolm wrote:
> On Fri, 2020-05-22 at 01:30 +0200, Mark Wielaard wrote:
> > This is on top of the stdbool.h and stdint.h patches.
> 
> Sorry, I didn't see those patches; I've replied to them now.

No worries, there was no hurry and I didn't CC you on those.
I pushed them both now. Thanks.

> > This adds a flag to c_parser so we know when we were trying to
> > constract a string literal. If there is a parse error and we were
> > constructing a string literal, and the next token is an unknown
> > identifier name, and we know there is a standard header that defines
> > that name as a string literal, then add a missing header hint to
> > the error messsage.
> 
> By comparison, what's the existing behavior for the example?
> Is it just what you posted below, but without the "note" diagnostics?

Yes.

> > I haven't checked yet if we can do something similar for the C++
> > parser. And the testcase needs to be extended a bit. But I hope the
> > direction is OK.
> 
> I think it's a worthy goal; as noted below I'd want a C frontend
> maintainer to approve those parts of it.

OK.

> > $ /opt/local/gcc/bin/gcc -c t.c
> > t.c:4:26: error: ‘PRIx64’ undeclared here (not in a function)
> >     4 | const char *hex64_fmt =  PRIx64;
> >       |                          ^~~~~~
> > t.c:3:1: note: ‘PRIx64’ is defined in header ‘<inttypes.h>’; did you forget to ‘#include <inttypes.h>’?
> >     2 | #include <stdint.h>
> >   +++ |+#include <inttypes.h>
> >     3 |
> > t.c:5:35: error: expected ‘,’ or ‘;’ before ‘PRIx64’
> >     5 | const char *msg_fmt = "Provide %" PRIx64 "\n";
> >       |                                   ^~~~~~
> > t.c:5:35: note: ‘PRIx64’ is defined in header ‘<inttypes.h>’; did you forget to ‘#include <inttypes.h>’?
> 
> It's a big improvement, but it's still a little clunky.  The
>   expected ‘,’ or ‘;’ before ‘PRIx64’
> is the sort of thing we've gotten used to with years of GCC messages,
> but might make little sense to a neophyte.
> I wonder if it's possible to improve this further, but I fear that that
> might overcomplicate things (if I understand things correctly, the
> string concatenation fails in the tokenizer, and then the PRIx64 fails
> in the parser, so we have a bad interaction involving two different
> levels of abstraction within the frontend - I think).

Yes, part of the string concatenation is done by the tokenizer, the
other part by the parser. With this patch we now track when we were
trying to concatenate a string. Then if any error occurs and we notice
an unknown token is next that could be a string literal then we add
the hint to the error message with an alternative suggestion/hint for
a fix. I didn't want to override the existing error message, because
it might be right or better, and we don't actually know whether with
the suggestion the rest will actully parse correctly.

> > +/* These can be used as string macros or as identifiers. Must all be
> > +   string-literals.  Used in get_stdlib_header_for_name and
> > +   get_c_stdlib_header_for_string_macro_name.  */
> > +static const stdlib_hint c99_cxx11_macro_hints[] = {
> > +    /* <inttypes.h> and <cinttypes>.  */
> > +    {"PRId8", {"<inttypes.h>", "<cinttypes>"} },
> > [...]
> > +    {"SCNxPTR", {"<inttypes.h>", "<cinttypes>"} }
> > +};
> 
> I found myself squinting at the array trying to decide if every entry
> had
>    {"<inttypes.h>", "<cinttypes>"}
> as its second element.  I *think* that's the case, right? 
> 
> I know there's a lot of pre-existing duplication in this file, but
> maybe this would be better as simply an array of const char *?
> It could probably be reformatted to take up far fewer lines by grouping
> them logically.
> 
> Maybe have a
> 
>   static const char *
>   get_c99_cxx11_macro_hint (const char *, enum stdlib lib);
> 
> to do the predicate, and return one of "<inttypes.h>", "<cinttypes>", or NULL?

Yes, that is actually better. And much easier to read. And the code
can still be shared between get_c_stdlib_header_for_string_macro_name
and get_stdlib_header_for_name. Changed in the attached patch.

I also extended the testcase so it covers both identifiers and string
concatenation cases. The identifier hints also work for C++, but I
haven't yet added the string token concatenation detection to the
cp_parser. It looks like it can be done similar as done for the
c_parser, but the parsers are different enough that it seems
better/simpler to do that in a followup patch once we have this patch
in the the c_parser.

Thanks,

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Provide-diagnostic-hints-for-missing-inttypes.h-stri.patch
Type: text/x-diff
Size: 18131 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200523/f69ba91a/attachment-0001.bin>