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