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