[PATCH] libcpp: Implement clang -Wheader-guard warning [PR96842]

[David Malcolm]

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