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