kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:35
+// Captures #include mapping information. It analyses IWYU Pragma comments and
+// other use-instead-like mechanisms (#pragma include_instead) on included
----------------
triple slashes (all over the header)
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:41
+// spelling header rather than the header directly defines the symbol.
+class PragmaIncludes {
+public:
----------------
i think this interface is OK, if we're planning to implement another adapter on
top of this class and use this only as a repository. Then have the adapter
provide a more convenient interface/data structures for types of queries we'll
have during `Location => Header` mapping inside include cleaner.
As those will most likely look like
`getHeaders(SourceLocation)/getHeaders(string)` -> `vector<pair<Header,
Signals>>`, without really caring about the details an implementation detail
being mapped to some other file, or getting exports of a certain header (as
discussed this is the piece that would require a little bit more detailed
design).
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:49
+ // `IWYU pragma: keep`.
+ bool shouldKeep(unsigned HashLineNumber) const;
+ // Returns the mapping include for the given physical header file.
----------------
it feels a little bit unfortunate from layering perspective that this struct
also needs to somehow have specialized knowledge about the main file, but also
having the same comment handler logic both in here and inside a main file
specific handler seems like too much duplication. So let's go with it in this
way for now, but keep this quirk in mind so that we can decide on a better
place in the future.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:54
+
+ class RecordPragma;
+
----------------
why do we need to expose this?
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:58
+ // Line numbers for the include directives in the main file that have the
+ // `IWYU pragma: keep` right after.
+ llvm::DenseSet</*0-indexed line number*/ unsigned> ShouldKeep;
----------------
we should also keep headers that're marked as `export`.
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:64
+ // We always insert using the spelling from the pragma.
+ if (auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin())))
+ Out->IWYUPrivate.insert(
----------------
nit: we can keep track of the current FileID using
`FileChanged/LexedFileChanged` events (also gets rid of the
`isWrittenInMainFile` call above/below).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136071/new/
https://reviews.llvm.org/D136071
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
