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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
