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