[PATCH] D134654: [clang] Detect header loops

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan marked 2 inline comments as done.
urnathan 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:
> 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?


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:324
+def warn_include_cycle : Warning<"#include cycle">,
+   InGroup<DiagGroup<"include-cycle">>, DefaultIgnore;
 
----------------
aaron.ballman wrote:
> We have a reasonable amount of evidence that off-by-default warnings remain off for basically everyone. Can we do anything to enable this diagnostic by default? e.g., would it make sense to enable it by default when trying to build a module but otherwise silence it?
That's probably a good idea -- I didnt want to put it unconditionally on, because it triggers all over the testsuite due to intentional self-inclusion


================
Comment at: clang/lib/Lex/PPLexerChange.cpp:107
+  if (const FileEntry *FE = SourceMgr.getFileEntryForID(FID)) {
+    auto [I, Added] = ActiveIncludedFiles.insert(FE);
+    if (!Added)
----------------
erichkeane wrote:
> The iterator is somewhat costly (at least 2 pointers?) and since we're not accessing them, copying them into the structured binding object probably isn't valuable?  
I'd be very disappointed it the optimizer can't see through that and elide the copies here. (but I had misremembered how  structured bindings are not references to the temporary by default anyway)


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

https://reviews.llvm.org/D134654