hokein added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:38
+// spelling header rather than the header directly defines the symbol.
+class PragmaIncludes {
+public:
----------------
sammccall wrote:
> sammccall wrote:
> > IWYU pragmas requires a PP listener, we'll also need PP events for other
> > purposes (determining which files are self-contained, finding #includes in
> > the main file). Should these be the same listener and write into the same
> > class?
> >
> > The argument for fewer components is usability: the embedder (standalone
> > tool, clangd, clang-tidy) is responsible for connecting these hooks to the
> > compiler and feeding the results back into the analysis.
> >
> > So the library is somewhat simpler to use if the number of parts is small.
> > We need (I think) at least one for the AST and one for the preprocessor,
> > because they need to be connected at different parts of the lifecycle.
> >
> > How much do we gain by splitting up further than that?
> After discussing offline: I think we expect this to be reused in clangd so
> separating it from the bits that won't is useful.
> I don't think the scope is exactly clear though: which of these should we
> bundle together?
>
> - IWYU pragmas related to headers (private, export)
> - IWYU pragmas related to the main file (keep/no_include)
> - other equivalent things like `#pragma include_instead`
> - tracking whether headers are self-contained
> - recording `#include` graph (not sure IWYU even needs this)
> - tracking macro occurrences in the main file
> - details of `#include` directives from the main file
>
> I'm tempted to say 1/2/3/4 are in-scope (since they're all gathered from
> roughly the same source and needed for the same thing) and so
> `PragmaIncludes` seems a little too narrow. Suggest `HeaderQuirks` maybe?
My original plan was 1/2/3 (4 probably yes, as they mostly share the same
thing).
As a personal preference, I like the name `PragmaIncludes` as it perfectly
matches 1/2/3, but not 4; while the `HeaderQuirks` is probably a better
umbrella, but it seems not quite self-contained. for now, I'd keep it as-is,
but happy to change if you have a strong preference.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:44
+
+ bool shouldKeep(FileEntryRef File) const;
+ // Returns the mapping include for the given physical header file.
----------------
sammccall wrote:
> At first glance, this signature doesn't look right: `keep` applies to
> directives, not to files.
> e.g.
> ```
> #include "foo.h" // keep
> #include "foo.h"
> ```
> the second should be removed
>
> (it's a silly example, but in the past we've confused ourselves into doing IO
> or complicated string matching code for things that are about spelling).
>
> I'd suggest some representation like the line number (where the `#` appears,
> not where the comment is), though the details probably matter less than the
> concept.
right, good point!
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:53
+ // Main file headers that should be kept, decorated by "IWYU pragma: keep".
+ llvm::DenseSet<llvm::sys::fs::UniqueID> ShouldKeep; // FIXME: implement
+
----------------
sammccall wrote:
> sammccall wrote:
> > Why are these UniqueID rather than FileEntry*? Is the idea to try to use
> > this in clangd?
> >
> > We hadn't planned on doing this, because pushing clangd's weird
> > requirements around not using FileEntry's to the library, ability to clone
> > the data structure etc seemed pretty intrusive. Since we switched from
> > name-based to UniqueID it might be doable...
> After discussing offline, using UniqueID to ensure the results are usable
> across FileManagers seems reasonable, but I think it needs to be documented.
Added comments.
================
Comment at: clang-tools-extra/include-cleaner/lib/Hooks.cpp:24
+static llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
+ constexpr llvm::StringLiteral IWYUPragma = "// IWYU pragma: ";
+ if (strncmp(Text, IWYUPragma.data(), IWYUPragma.size()))
----------------
sammccall wrote:
> while we're adding this, we should probably support `/*` as well as `//`,
> it's allowed per spec (I didn't add it to clangd just to keep NFC)
added handling of `/*`
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
