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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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;
----------------
kadircet wrote:
> 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.
this is a good point. Thinking more about it, I think we don't even need `RealPathNamesByUID`at all, if we store the Path as the value. 


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:64
+    if (!ExportStack.empty() &&
+        ExportStack.back().Exporter == SM.getFileID(HashLoc)) {
+      auto Top = ExportStack.back();
----------------
kadircet wrote:
> 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).
yeah, this is an invalid case, I think we should not put too much effort on handling all invalid cases, but this one is trivial to support, added line-number to the `State`.


================
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(
----------------
kadircet wrote:
> 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?
right, but with the new implementation, we use the # line number to make sure the #include is within the export range, this logic needs to be here, otherwise the semantic of `ShouldKeep` will change to `track all lines that have IWYU pragmas, even for lines without #include directive`.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:110
+      if (!ExportStack.empty()) {
+        assert(ExportStack.back().Block);
+        ExportStack.pop_back();
----------------
kadircet wrote:
> can you also assert that Stack top and current Range are from the same file id?
this assert makes sense for matching begin/end_exports case, but it is too strong for invalid cases, as the current code doesn't proper recover the unmatched begin/end exports cases at the moment (we might want to add some basic recovery mechanisms, but I think it is not a high priority).

e.g. for the case

good.h
```
// IWYU pragma: begin_exports
#include "bad.h"
// IWYU pragma: end_exports
```

bad.h

```
// IWYU pragma: end_exports
```


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:144
   int LastPragmaKeepInMainFileLine = -1;
+  struct State {
+    // The file where we saw the export pragma.
----------------
kadircet wrote:
> maybe call this `ExportPragma` instead of `State`?
ExportPragma seems more confusing than State, it doesn't express that we're tracking the state of the export pragma. 


================
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
----------------
kadircet wrote:
> what about just `Location` or `SeenAt`?
renamed to `SeenAt`.


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:146
+
+  // Exports
+  EXPECT_TRUE(PI.shouldKeep(5));
----------------
kadircet wrote:
> i think we should also EXPECT_TRUE for line 6 and 9.
for these two cases it is false, based on the semantic of `ShouldKeep`, there is no #include directive on Line 6&9, so line 6&9 is not in the set, it will return false.

If there are #include directives on these lines, these are corner cases, multiple combinations:

- `/* IWYU Pragma: begin_exports*/ #include "keep.h"`
- `#include "nokeep.h" /*IWYU Pragma: begin_exports*/`
- `/* IWYU Pragma: end_exports*/ #include "nokeep.h"`
- `#include "keep.h" /* IWYU Pragma: end_exports*/`

Fully supporting these requires some effort and code complexity, and I believe these are rare cases, I would ignore these cases for now.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137319