hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+ // FIXME: this map should probably be in IncludeStructure.
+ llvm::StringMap<llvm::SmallVector<IncludeStructure::HeaderID>> BySpelling;
----------------
kadircet wrote:
> agreed, let's change `StdlibHeaders` in `IncludeStructure` to actually be a
> `BySpelling` mapping, it should be no-op for existing implementation as we're
> only checking for stdlib headers already (and there's no other usage)
sure, I will address it in a followup patch.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+ }
+ // FIXME: !!this is a hacky way to collect macro references.
+ std::vector<include_cleaner::SymbolReference> Macros;
----------------
kadircet wrote:
> this might behave slightly different than what we have in `RecordedPP`, and
> rest of the applications of include-cleaner will be using that. can we expose
> the pieces in RecordedPP that collects MacroRefs as a separate handler and
> attach that collector (combined with the skipped range collection inside
> `CollectMainFileMacros` and also still converting to `MainFileMacros` in the
> end (as we can't store sourcelocations/identifierinfos from preamble)?
>
> afterwards we can use the names stored in there to get back to
> `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?
Yeah, this is a better solution, but I'm not sure whether we should do this
before the release cut, it has a side effect of changing the
find-macro-reference behavior in clangd. It requires some design/implement work.
I agree that the current solution is hacky, and eventually will be replaced,
but it follows the existing `findReferencedMacros`, so it is not that bad. I
tend to land this version before the release cut. What do you think?
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:487
+ Macros.push_back({Tok.location(),
+ include_cleaner::Macro{/*Name=*/nullptr, DefLoc},
+ include_cleaner::RefType::Explicit});
----------------
kadircet wrote:
> you can get the `IdentifierInfo` using `Name` inside `Macro` and PP.
oh, right. Done.
================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+ Config::UnusedIncludesPolicy::Experiment)
+ Pragmas.record(*Clang);
// Copy over the macros in the preamble region of the main file, and combine
----------------
kadircet wrote:
> why do we need to collect pragmas in main file? i think we already have
> necessary information available via `IncludeStructure` (it stores keeps and
> exports, and we don't care about anything else in the main file AFAICT). so
> let's just use the PragmaIncludes we're getting from the Preamble directly?
> without even making a copy and returning a reference from the `Preamble`
> instead in `ParsedAST::getPragmaIncludes`
> i think we already have necessary information available via IncludeStructure
> (it stores keeps and exports, and we don't care about anything else in the
> main file AFAICT)
The IncludeStructure doesn't have a full support for IWYU export pragma, it
only tracks the headers that have the export pragma.
My understand of the end goal is to use the `PragmaInclude` to handle every
IWYU-related things, and we can remove all these IWYU bits in the
`IncludeStructure`, clangd IWYU pragma handling
[code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140875/new/
https://reviews.llvm.org/D140875
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
