Header guard warning is too agressive

[ismailp]

Summary

If the edit distance between the two macros is more than 50%, DefinedMacro may not be header guard, or can be header guard of another header file, or it might be defining something completely different. This can be observed in the wild when handling feature macros or header guards in different files.

// foo.c
#ifndef NO_FLOATING_POINT_SUPPORT
#define USE_SOMETHING_ELSE
#include "foo.h"
/* ... */
#endif /* !defined(NO_FLOATING_POINT_SUPPORT)

[ismailp]

Ping! This also fixes PR17053.

lib/Lex/PPLexerChange.cpp

293

I haven't tried thoroughly, but I guess it'd be better to print the fix-it on the warning instead of the note, because clang-check will ignore the note (and I need clang-check to apply the fixit).

[rsmith]

rtrieu would be a more appropriate reviewer for this.

[rtrieu]

If I understand the issue correctly, and from reading PR17053, the problem is that the main file in the translation unit would have a structure similar to a header guard but it should not be considered a header guard. This patch does not fix that issue. Instead, it looks at the edit distance between #define and #ifndef macro names. This only coincidentally fixes the examples given. The test case also does not follow the examples as it is still inside a header, not the main file.

It would be better to detect if the lexer is at the top most file and skip the diagnostic then.

lib/Lex/PPLexerChange.cpp

293

This seems like a valid point. There is only one suggestion, so it should be placed in the warning instead of the note, but that change should go in a separate patch.

[krememek]

This warning is turning out to be quite noisy. The edit distance heuristic, while not a complete fix for PR17053, would potentially help make this warning more useful in practice regardless.

For example, I'm seeing a warning on the following code:

#ifndef NO_SYSCALL_LEGACY

#define _NONSTD_SOURCE`

The edit distance heuristic would not warn here. Even if the edit distance heuristic doesn't solve the complete problem, it would be good to make some incremental progress here.

[rtrieu]

I have no objections to this patch. Keep PR17053 open for now. If you fix the indenting issues I marked, this should be ready for committing.

lib/Lex/PPLexerChange.cpp

278

Add a newline after the "=" and indent four spaces.

286

Indent all the "<<" operators two spaces.

294

80 column violation. Indent these lines 4 past the start of FixItHint

[ismailp]

Except line 277, (const size_t MaxHalfLength = std::max...), it's the output from clang-format within range 268:296. Update addresses Richard's comments.

I have seen this warning fired in legacy code. Our case is quite similar to Ted's example. PR17053 is filed after I submitted this. Please don't consider this patch as a fix for PR17053. It seems like this patch, as Richard said previously, coincidentally fixes PR17053 as well.

[ismailp]

Committed in r192547.

[ismailp]

Committed in r192547 (should have been "Close revision", but it wasn't accepted to be closed).

lib/Lex/PPLexerChange.cpp

if (const IdentifierInfo *ControllingMacro =
       if (const FileEntry *FE =
             SourceMgr.getFileEntryForID(CurPPLexer->getFileID())) {
         HeaderInfo.SetFileControllingMacro(FE, ControllingMacro);
         if (const IdentifierInfo *DefinedMacro =
               CurPPLexer->MIOpt.GetDefinedMacro()) {
           if (!ControllingMacro->hasMacroDefinition() &&
               DefinedMacro != ControllingMacro &&
               HeaderInfo.FirstTimeLexingFile(FE)) {
+
+            // If the edit distance between the two macros is more than 50%,
+            // DefinedMacro may not be header guard, or can be header guard of
+            // another header file. Therefore, it maybe defining something
+            // completely different. This can be observed in the wild when
+            // handling feature macros or header guards in different files.
+
+            const StringRef ControllingMacroName = ControllingMacro->getName();
+            const StringRef DefinedMacroName = DefinedMacro->getName();
+            const size_t MaxHalfLength = std::max(ControllingMacroName.size(),
+                                                  DefinedMacroName.size()) / 2;
+            const unsigned ED = ControllingMacroName.edit_distance(
+                DefinedMacroName, true, MaxHalfLength);
+            if (ED <= MaxHalfLength) {
               // Emit a warning for a bad header guard.
               Diag(CurPPLexer->MIOpt.GetMacroLocation(),
                    diag::warn_header_guard)
-                << CurPPLexer->MIOpt.GetMacroLocation()
-                << ControllingMacro;
+                  << CurPPLexer->MIOpt.GetMacroLocation() << ControllingMacro;
               Diag(CurPPLexer->MIOpt.GetDefinedLocation(),
                    diag::note_header_guard)
-                << CurPPLexer->MIOpt.GetDefinedLocation()
-                << DefinedMacro
+                  << CurPPLexer->MIOpt.GetDefinedLocation() << DefinedMacro
                   << ControllingMacro
                   << FixItHint::CreateReplacement(
                          CurPPLexer->MIOpt.GetDefinedLocation(),
                          ControllingMacro->getName());
             }
           }
         }
       }
     }
+  }
 
   // Complain about reaching a true EOF within arc_cf_code_audited.
   // We don't want to complain about reaching the end of a macro
   // instantiation or a _Pragma.
   if (PragmaARCCFCodeAuditedLoc.isValid() &&
       !isEndOfMacro && !(CurLexer && CurLexer->Is_PragmaLexer)) {
     Diag(PragmaARCCFCodeAuditedLoc, diag::err_pp_eof_in_arc_cf_code_audited);
 

test/Lexer/Inputs/unlikely-to-be-header-guard.h

+#ifndef data_rep_h
+#define use_alternate_data_rep
+/* #include "data_rep.h" */
+#endif
+

test/Lexer/header.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wno-header-guard %s
 // RUN: %clang_cc1 -fsyntax-only -Wheader-guard %s 2>&1 | FileCheck %s
 
 #include "Inputs/good-header-guard.h"
 #include "Inputs/no-define.h"
 #include "Inputs/different-define.h"
 #include "Inputs/out-of-order-define.h"
 #include "Inputs/tokens-between-ifndef-and-define.h"
+#include "Inputs/unlikely-to-be-header-guard.h"
 
 #include "Inputs/bad-header-guard.h"
 // CHECK: In file included from {{.*}}header.cpp:{{[0-9]*}}:
 // CHECK: {{.*}}bad-header-guard.h:1:9: warning: 'bad_header_guard' is used as a header guard here, followed by #define of a different macro
 // CHECK: {{^}}#ifndef bad_header_guard
 // CHECK: {{^}}        ^~~~~~~~~~~~~~~~
 // CHECK: {{.*}}bad-header-guard.h:2:9: note: 'bad_guard' is defined here; did you mean 'bad_header_guard'?
 // CHECK: {{^}}#define bad_guard