kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:100
+ /// Contains all non self-contained files during the parsing.
+ llvm::DenseSet<llvm::sys::fs::UniqueID> NonSelfContainedFiles;
----------------
s/during/detected during/
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:18
+// up the include stack.
+static const FileEntry *getResponsibleHeader(FileID FID,
+ const SourceManager &SM,
----------------
what about `selfContainedHeader` instead of `getResponsibleHeader`,
responsibility is a concept that we rather define between which header should
include what headers (e.g. a header providing return types of its APIs)
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:24
+ while (FE && FID != SM.getMainFileID() && !PI.isSelfContained(FE)) {
+ FID = SM.getFileID(SM.getIncludeLoc(FID));
+ FE = SM.getFileEntryForID(FID);
----------------
`FID = SM.getDecomposedIncludedLoc(FID).first;`
which does a cached lookup, so might be faster than `getFileID`, especially
when performing repeated lookups (e.g. a single file importing multiple
tablegen'd headers).
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:37
// FIXME: Handle macro locations.
// FIXME: Handle non self-contained files.
FileID FID = SM.getFileID(Loc.physical());
----------------
drop fixme
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:38
// FIXME: Handle non self-contained files.
FileID FID = SM.getFileID(Loc.physical());
+ const auto *FE = getResponsibleHeader(FID, SM, PI);
----------------
can you drop `FID` (as it isn't used elsewhere) and just pass `Loc.physical()`
into helper?
================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:39
FileID FID = SM.getFileID(Loc.physical());
- const auto *FE = SM.getFileEntryForID(FID);
+ const auto *FE = getResponsibleHeader(FID, SM, PI);
if (!FE)
----------------
i am not sure if sequencing of this and rest of the IWYU pragma handling is the
right actually.
e.g. consider a scenario in which:
private.h:
```
// IWYU private, include "public.h"
void foo();
```
private-impl.h:
```
#pragma once
#include "private.h"
```
we'll actually now use `private-impl.h` as provider for `foo` rather than
`public.h`, a similar argument goes for export pragma. i think intent of the
developer is a lot more explicit when we have these pragmas around, so we
should prioritize them and then pick the self contained one, if it's still
needed. WDYT?
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:306
+bool PragmaIncludes::isSelfContained(const FileEntry *ID) const {
+ return !NonSelfContainedFiles.contains(ID->getUniqueID());
----------------
s/ID/File
================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:31
+std::string guard(const llvm::StringRef Code) {
+ return "#pragma once\n" + Code.str();
----------------
drop the `const`
================
Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:20
+std::string guard(const llvm::StringRef Code) {
+ return "#pragma once\n" + Code.str();
----------------
again `const`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137698/new/
https://reviews.llvm.org/D137698
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
