[PATCH] D134654: [clang] Detect header loops

[Nikolas Klauser via Phabricator via cfe-commits]

philnik added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:323
   "whitespace recommended after macro name">;
+def warn_include_cycle : Warning<"#include cycle">,
+   InGroup<DiagGroup<"include-cycle">>, DefaultIgnore;
----------------
aaron.ballman wrote:
> urnathan wrote:
> > aaron.ballman wrote:
> > > This diagnostic doesn't really seem like it's going to help the user to know what's wrong with their code or how to fix it. (Also, the notes @erichkeane was hoping were emitted don't seem to be, at least according to the new test cases.) I think we need to give users a bit more guidance here.
> > The test infrastructire ignores the 'included from ' chain, which is emitted.  What do you suggest?
> Even posting the "included from" doesn't help clarify what's wrong with the code though, if I'm understanding the tests properly. My concern is with code like:
> ```
> // self.h
> #ifndef GUARD
> #define GUARD
> 
> #include "self.h" // expected-warning {{#include cycle}}
> #endif
> ```
> (This is effectively the same test as `warn-loop-macro-1.h`.) There is no include *cycle* ("a series of events that are regularly repeated in the same order.") in this code -- the header is included for the first time, `GUARD` is not defined, then `GUARD` is defined and the header is included for the second time. This time `GUARD` is defined and no further cycles into `self.h` occurs.
> 
> But I think I'm seeing now what you're trying to get at (correct me if I'm wrong): when processing an include stack, opening the same header file twice is a problem (regardless of the contents of the header or how you got into the same file twice)? If that's accurate, would it make more sense to diagnose this as:
> 
> `including the same header (%0) more than once in an include stack causes %select{incorrect behavior of Clang modules|the header to not be a C++ module header unit}1`
> 
> or something along those lines? Basically, I'd like to avoid saying "cycle" because that starts to sound like "you have potentially infinite recursion in the include stack" and I'd like to add information about what's actually wrong instead of pointing out what the code does and hoping the user knows why that's bad.
FWIW in libc++ we have a script to check that we don't include the header more than once in a stack and also call it a cycle, so there is some precedent of calling it an include cycle, even thought it's technically not a cycle.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134654/new/

https://reviews.llvm.org/D134654