VitaNuo marked 19 inline comments as done.
VitaNuo added a comment.
Thank you!
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:839
+ if (!Headers.empty())
+ SymbolProviders.insert({S.ID, Headers[0]});
}
----------------
kadircet wrote:
> once we have the optional<Header> as the value type you can also rewrite this
> as:
> ```
> auto [It, Inserted] = SymbolProviders.try_emplace(S.ID);
> if (Inserted) {
> auto Providers = include_cleaner::headersForSymbol(Sym, SM,
> Opts.PragmaIncludes.get());
> if(!Providers.empty()) It->second = Providers.front();
> }
> ```
>
> that way we can get rid of some redundant calls to `headersForSymbol`
Sure, although I'm not sure where the redundant calls would be coming from.
I've been under the impression that this function is supposed to be called once
for each symbol.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:913
+ if (!Inserted)
+ continue;
+ switch (H.kind()) {
----------------
kadircet wrote:
> we shouldn't `continue` here, it just means we can directly use `SpellingIt`
> to figure out what to put into `NewSym.IncludeHeaders`.
>
> maybe something like:
>
> ```
> auto [SpellingIt, Inserted] = HeaderSpelling.try_emplace(H);
> if (Inserted)
> SpellingIt-> second = getSpelling(H, *PP);
> if(!SpellingIt->second.empty()) {
> Symbol NewSym = *S;
> NewSym.IncludeHeaders.push_back({SpellingIt->second, 1, Directives});
> Symbols.insert(NewSym);
> }
> ```
>
> ```
> std::string getSpelling(const include_cleaner::Header &H, const Preprocessor
> &PP) {
> if(H.kind() == Physical) {
> // Check if we have a mapping for the header in system includes.
> // FIXME: get rid of this once stdlib mapping covers these system headers
> too.
> CanonicalIncludes CI;
> CI.addSystemHeadersMapping(PP.getLangOpts());
> if (auto Canonical = CI.mapHeader(H.physical()->getLastRef());
> !Canonical.empty())
> return Canonical;
> if (!tooling::isSelfContainedHeader(...))
> return "";
> }
> // Otherwise use default spelling strategies in include_cleaner.
> return include_cleaner::spellHeader(H);
> }
> ```
As agreed offline, it is not easy to split out a spelling function, since
`HeaderFileURIs` is a private member of the symbol collector, and we seem to
still need to generate URIs for physical headers going forward.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:877
// We delay this until end of TU so header guards are all resolved.
for (const auto &[SID, FID] : IncludeFiles) {
if (const Symbol *S = Symbols.find(SID)) {
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > let's iterate over `SymbolProviders` instead, as `IncludeFiles` is a
> > > legacy struct now.
> > Not sure I'm following. Iterating over `SymbolProviders` means retrieving
> > `include_cleaner::Header`s. How would I handle the entire Obj-C part then,
> > without the FID of the include header?
> we're joining `IncludeFiles` and `SymbolProviders`, as they have the same
> keys.
>
> right now you're iterating over the keys in `IncludeFiles` and doing a lookup
> into `SymbolProviders` using that key to get providers.
> we can also do the iteration over `SymbolProviders` and do the lookup into
> `IncludeFiles` instead to find associated `FID`.
Ok, makes sense now, thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152900/new/
https://reviews.llvm.org/D152900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
