kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/Headers.cpp:134 + return false; + return inMainFile() ? handleCommentInMainFile(PP, Range) + : handleCommentInHeaderFile(PP, Range); ---------------- the split seems to be increasing code duplication (we've already copied a bug with it 😓), what about: ``` bool Err = false; llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err); if (!Err || (!Text.consume_front(IWYUPragmaKeep) && !Text.consume_front(IWYUPragmaExport))) return false; if(inMainFile()) { unsigned Offset = SM.getFileOffset(Range.getBegin()); LastPragmaKeepInMainFileLine = SM.getLineNumber(SM.getMainFileID(), Offset) - 1; } else { Out->HasIWYUPragmas.insert( *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin())))); } return false; ``` ================ Comment at: clang-tools-extra/clangd/Headers.cpp:154 + bool handleCommentInMainFile(Preprocessor &PP, SourceRange Range) { + assert(inMainFile()); + assert(!Range.getBegin().isMacroID()); ---------------- i don't think the asserts carry their weight (here and also in the headerfile comment handler). we're not really performing anything dubious if the asserts don't hold. getCharacterData will return non-sense (but valid) buffers in case of a macroid, and we're already reading data whether we're in the main file or not. moreover these are private and controlled at the entry by the public interface (`HandleComment`). ================ Comment at: clang-tools-extra/clangd/Headers.cpp:172 + llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err); + if (Err && !Text.consume_front(IWYUPragmaExport) && + !Text.consume_front(IWYUPragmaBeginExports)) ---------------- shouldn't this be `if (Err || (!Text.consume ... && !Text.consume...)) return false;` ? same above for the main file comment handling ================ Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:271 } // Headers without include guards have side effects and are not // self-contained, skip them. ---------------- can you also move this comment below, next to the `isFileMultipleHeaderGuarded` check? ================ Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:427 +)cpp"; + FS.Files[testPath("export.h")] = R"cpp( +#pragma once ---------------- no need for `testPath`s here and below. ================ Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:438 + FS.Files[testPath("none.h")] = R"cpp( +#pragma once +)cpp"; ---------------- this is passing not because you don't have a IWYU pragma comment here, but because you don't have any comments at all. can you try inserting an arbitrary comment into the file? ================ Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:603 + EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty())); + // FIXME: This is not correct: foo.h and bar.h are unused but are not + // diagnosed as such because we ignore headers with IWYU export pragmas for ---------------- do you mean just `foo.h is unused` why are we talking about `bar.h` also being unused here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125468/new/ https://reviews.llvm.org/D125468 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits