On Wed, 2024-09-11 at 23:26 +0200, Jakub Jelinek wrote:
> Hi!
>
> The following patch implements the clang -Wheader-guard warning,
> which warns
> if a valid multiple inclusion header guard's #ifndef/#if !defined
> directive
> is immediately (no other non-line directives nor other (non-comment)
> tokens in between) followed by #define directive for some different
> macro,
> which in get_suggestion rules is close enough to the actual header
> guard
> macro (i.e. likely misspelling), the #define is object-like with
> empty
> definition (I've followed what clang implements) and the macro isn't
> defined
> later on (at least not on the final #endif at the end of a header).
>
> In this case it emits a warning, so that
> #ifndef STDIO_H
> #define STDOI_H
> ...
> #endif
> or similar misspellings can be caught.
>
> clang enables this warning by default, but I've put it into -Wall
> instead
> as it still seems to be a style warning, nothing more severe; if a
> header
> doesn't survive multiple inclusion because of the misspelling, users
> will
> get different diagnostics.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
Overall, LGTM, but I'm not as familiar with libcpp's implementation
details.
>
> --- libcpp/files.cc.jj 2024-09-03 16:47:47.322031849 +0200
> +++ libcpp/files.cc 2024-09-11 19:32:43.754868132 +0200
> @@ -1664,7 +1664,28 @@ _cpp_pop_file_buffer (cpp_reader *pfile,
> /* Record the inclusion-preventing macro, which could be NULL
> meaning no controlling macro. */
> if (pfile->mi_valid && file->cmacro == NULL)
> - file->cmacro = pfile->mi_cmacro;
> + {
> + file->cmacro = pfile->mi_cmacro;
> + if (pfile->mi_cmacro
> + && pfile->mi_def_cmacro
> + && pfile->cb.get_suggestion)
> + {
> + const char *names[]
> + = { (const char *) NODE_NAME (pfile->mi_def_cmacro),
> NULL };
> + if (pfile->cb.get_suggestion (pfile,
> + (const char *)
> + NODE_NAME (pfile-
> >mi_cmacro), names)
> + && cpp_warning_with_line (pfile, CPP_W_HEADER_GUARD,
> + pfile->mi_loc, 0,
> + "header guard \"%s\"
> followed by "
> + "\"#define\" of a different
> macro",
> + NODE_NAME (pfile-
> >mi_cmacro)))
> + cpp_error_at (pfile, CPP_DL_NOTE, pfile->mi_def_loc,
> + "\"%s\" is defined here; did you mean
> \"%s\"?",
> + NODE_NAME (pfile->mi_def_cmacro),
> + NODE_NAME (pfile->mi_cmacro));
> + }
We were chatting on IRC about how it would be nice to be able to use
%qs in libcppp diagnostics; here is an example (rather than using
\"%s\").
Not a blocker, but it occurs to me that ideally we'd group the warning
and note into a diagnostic group, but unfortunately there's no way to
express that currently via the interface libcpp has. We would need to
add {begin,end}_group hooks, which in turn suggests that maybe that
libcpp's interface into diagnostics should be an abstract base class
with various vfuncs, rather than a callback.
Also not a blocker, but it would nice to have a fix-it hint here, by
using the rich_location overload of cpp_error_at and adding a fix-it
hint to the rich_location.
Hope this is constructive
Dave
