[PATCH] D137319: [include-cleaner] Add export IWYU pragma support.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:82
+  llvm::DenseMap<llvm::sys::fs::UniqueID,
+                 llvm::DenseSet<llvm::sys::fs::UniqueID>>
+      IWYUExportBy;
----------------
what about a smallvector, instead of a denseset here?


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:82
+  llvm::DenseMap<llvm::sys::fs::UniqueID,
+                 llvm::DenseSet<llvm::sys::fs::UniqueID>>
+      IWYUExportBy;
----------------
kadircet wrote:
> what about a smallvector, instead of a denseset here?
nit: instead of `UniqueID`s, you can directly store `StringRef`s in the values, if you use an Arena/StringSaver for storing full paths. that should save you from doing one extra lookup at query time.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:86
+  /// parsing.
+  llvm::DenseMap<llvm::sys::fs::UniqueID, std::string /*RealPath*/>
+      RealPathNamesByUID;
----------------
i think it's important to mention why we're storing a string to the full path here:
`There's no way to get a path for a UniqueID, especially when it hasn't been opened before. So store the full path and convert it to a FileEntry by opening the file again through a FileManager.`


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:64
+    if (!ExportStack.empty() &&
+        ExportStack.back().Exporter == SM.getFileID(HashLoc)) {
+      auto Top = ExportStack.back();
----------------
this has the nasty downside of:
```
// IWYU pragma: export
void foo();
#include "bar.h"
```

now bar.h will be considered exported :/ i think we have to store the line-number for the pragma as well and compare it (we can store `0` for block comments).


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:69
+      // main-file #include with export pragma should never be removed.
+      if (Top.Exporter == SM.getMainFileID())
+        Out->ShouldKeep.insert(
----------------
well, i've just noticed we're not actually doing anything specific to the include here (or for the `keep` equivalent) as we're just storing the line number we've seen the comment on.
so what about moving this logic (and the pragma keep one) into the `HandleComment` callback instead?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:110
+      if (!ExportStack.empty()) {
+        assert(ExportStack.back().Block);
+        ExportStack.pop_back();
----------------
can you also assert that Stack top and current Range are from the same file id?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:144
   int LastPragmaKeepInMainFileLine = -1;
+  struct State {
+    // The file where we saw the export pragma.
----------------
maybe call this `ExportPragma` instead of `State`?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:146
+    // The file where we saw the export pragma.
+    FileID Exporter;
+    // true if it is a block begin/end_exports pragma; false if it is a
----------------
what about just `Location` or `SeenAt`?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:179
+      auto FE = FM.getFileRef(RealExport->getSecond());
+      assert(FE);
+      Results.push_back(FE.get());
----------------
i think this is also a harsh assert. the file might get deleted from disk between indexing (e.g. clangd's preamble build) and query (e.g. clangd's AST built). so maybe log this occurrence instead and continue?


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:136
   )cpp";
-  Inputs.ExtraFiles["keep1.h"] = Inputs.ExtraFiles["keep2.h"] = "";
+  Inputs.ExtraFiles["keep1.h"] = Inputs.ExtraFiles["keep2.h"] =
+      Inputs.ExtraFiles["export1.h"] = Inputs.ExtraFiles["export2.h"] =
----------------
nit: indentation looks weird, maybe:
```
for(llvm::StringRef File : {"keep1.h", ...}) {
  Inputs.ExtraFiles[File] = "";
}
```


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:146
+
+  // Exports
+  EXPECT_TRUE(PI.shouldKeep(5));
----------------
i think we should also EXPECT_TRUE for line 6 and 9.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:193
+                                            FileNamed("export3.h")));
+  EXPECT_TRUE(PI.getExporters(FM.getFile("export3.h").get(), FM).empty());
+}
----------------
can you also assert that for export1, export2 and mainfile?


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:218-220
+  Inputs.ExtraFiles["private1.h"] = Inputs.ExtraFiles["private2.h"] =
+      Inputs.ExtraFiles["private3.h"] = "";
+  Inputs.ExtraFiles["foo.h"] = Inputs.ExtraFiles["bar.h"] = "";
----------------
nit: same indentation comment as above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137319