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