aaron.ballman 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;
----------------
urnathan wrote:
> urnathan wrote:
> > philnik wrote:
> > > 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.
> > Aaron, correct, the problem is with clang-modules/c++ header units. The
> > loop means that the header file does not expand to the same sequence of
> > tokens at every inclusion. This is particularly problematic with module
> > maps, because the compiler will automatically start turning a header into a
> > clang-module upon the first #include -- which might not be at the outermost
> > level, (if it starts with a header at some other position in the loop).
> > Then when it starts with this header, it'll think it already has converted
> > and loaded it. One ends up with very obscure failure modes (such as link
> > errors concerning missing template instantations).
> >
> > I wonder if (in addition to allow detecting this in general), the warning
> > should be enabled by default when building a header-unit/clang-module and
> > one reincludes a header corresponding to a module currently being built.
> > That's the same check, because there can be a stack of modules being built,
> > but I think we need some additional checking to avoid triggering on
> > 'textual headers' from the module map.
> >
> > IMHO it is still a cycle, it's a closed loop that executes exactly once :)
> > I can see the confusion though. I mean, 'for (auto i = 0; i < 1; i++) {}'
> > is still a loop, right? just not an unbounded one.
> >
> > Anyway, I think your diagagnostic text is better, thanks!
> philnik, that's good to know.
>
Thank you for the help understanding the root of what we're diagnosing! That's
fascinating and something I'm really glad we're getting a diagnostic for! I'm
more strongly in favor of this being an on-by-default warning that only
diagnoses when trying to build a module (if possible).
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
