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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
