[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/lib/Record.cpp:69
+      // main-file #include with export pragma should never be removed.
+      if (Top.Exporter == SM.getMainFileID())
+        Out->ShouldKeep.insert(
----------------
hokein wrote:
> 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`.
> `track all lines that have IWYU pragmas, even for lines without #include directive`

i am not sure what's so bad about this. i think it'll make the code less complicated, as inclusion directive doesn't need to care about mutating `ShouldKeep` anymore, and public api for the class doesn't actually care about include-directives for `shouldKeep` operation, it just operates on lines. so saying "yes" to preserving lines that have `// IWYU keep/export` but no associated inclusion directive, doesn't seem so confusing (i agree, it'd be better not to have, but i don't think is an important enough use case, i.e. people trying to make decisions for lines that don't have includes via include-cleaner is unlikely and possibly wrong enough that we shouldn't care).

up to you though, especially if you extract the export handling into a separate method, this shouldn't be too much of an issue.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:144
   int LastPragmaKeepInMainFileLine = -1;
+  struct State {
+    // The file where we saw the export pragma.
----------------
hokein wrote:
> 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. 
i think `State` doesn't emphasize the fact that it's about `export`s specifically, hence i was trying to suggest including `Export` in the name somehow.

moreover, i don't think we're tracking the state of the export pragma, we're literally recording occurrence of an export pragma, the state is rather tracked via `ExportStack` itself. hence i'd still suggest renaming this to `ExportPragma` and if you want change the `ExportStack` to `ExportState`, but i think `ExportStack` is fine as is.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:67
+
+    if (!ExportStack.empty() && ExportStack.back().SeenAtFile == HashFID) {
+      auto Top = ExportStack.back();
----------------
do you mind extracting this into a method and re-flow in a early-exit friendly way, it's getting a little bit hard to follow. e.g:
```
void checkForExport(FileID IncludingFile, FileEntryRef IncludedFile) {
  if (ExportStack.etmpy()) return;
  auto &Top = ExportStack.top();
  if (Top.SeenAtFile != IncludingFile) return;
  ...
}
```


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:68
+    if (!ExportStack.empty() && ExportStack.back().SeenAtFile == HashFID) {
+      auto Top = ExportStack.back();
+      if ((Top.Block && HashLine > Top.SeenAtLine) ||
----------------
nit: `auto &Top = ...` to prevent the copy


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:71
+          Top.SeenAtLine ==
+              HashLine) { // make sure the #include is within the export range.
+        Out->IWYUExportBy[File->getUniqueID()].push_back(
----------------
can you move the comment to be above the `if` statement instead? nesting seems weird here. we can say `make sure current include is covered by the export pragma`.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:72
+              HashLine) { // make sure the #include is within the export range.
+        Out->IWYUExportBy[File->getUniqueID()].push_back(
+            save(SM.getFileEntryForID(Top.SeenAtFile)->tryGetRealPathName()));
----------------
unfortunately `File` is `Optional`.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:73
+        Out->IWYUExportBy[File->getUniqueID()].push_back(
+            save(SM.getFileEntryForID(Top.SeenAtFile)->tryGetRealPathName()));
+        // main-file #include with export pragma should never be removed.
----------------
nit: i'd actually make `FullPath` part of `State` and use it directly from there, rather than possibly calling `tryGetRealPathName` and `save` multiple times here.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:105
+                                       SM.getFileOffset(Range.getBegin()));
+    // Handle export pragma.
+    if (Pragma->startswith("export")) {
----------------
nit: s/Handle/Record


================
Comment at: clang-tools-extra/include-cleaner/unittests/RecordTest.cpp:203
+  EXPECT_TRUE(PI.getExporters(FM.getFile("export1.h").get(), FM).empty());
+  EXPECT_TRUE(PI.getExporters(FM.getFile("export3.h").get(), FM).empty());
+  EXPECT_TRUE(PI.getExporters(FM.getFile("export2.h").get(), FM).empty());
----------------
nit: re-order export3 and export2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137319