[PATCH] D125468: [clangd] Include Cleaner: ignore headers with IWYU export pragmas

[Kadir Cetinkaya via Phabricator via cfe-commits]

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