This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Replace the hacky include-cleaner macro-reference implementation.
ClosedPublic

Authored by hokein on Mar 28 2023, 2:28 AM.

Details

Summary

Now MainFileMacros preserves enough information, we perform a just-in-time
convertion to interop with include-cleaner::Macro for include-cleaer features.

Diff Detail

Event Timeline

hokein created this revision.Mar 28 2023, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 2:28 AM
Herald added a subscriber: arphaman. · View Herald Transcript
hokein requested review of this revision.Mar 28 2023, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2023, 2:28 AM

Friendly ping in case you missed it.

thanks!

clang-tools-extra/clangd/IncludeCleaner.cpp
128

nit: const auto &[SID, Refs]

130

it's unfortunate that we're scanning the file for every occurrence of a macro just to get an offset. this might get problematic in big test files, what about introducing an offset to MacroOccurence?

135

nit: early exit instead

141

we should use Macro->NameLoc, not the definition loc inside macro-info

147

i believe you meant Ambigious here. the reference is "explicitly" in the code, but we can't say for sure if it's intended.

hokein updated this revision to Diff 532598.Jun 19 2023, 4:07 AM
hokein marked 3 inline comments as done.

address comments

thanks for the comments.

clang-tools-extra/clangd/IncludeCleaner.cpp
130

it's unfortunate that we're scanning the file for every occurrence of a macro just to get an offset. this might get problematic in big test files, what about introducing an offset to MacroOccurence?

130

thanks, it makes sense, sent out https://reviews.llvm.org/D153259.

141

oh, right. NameLoc takes the preamble patch into account.

kadircet accepted this revision.Jun 23 2023, 4:33 AM
kadircet added inline comments.
clang-tools-extra/clangd/IncludeCleaner.cpp
360

nit: auto Loc = SM.getComposedLoc(MainFile, Offset)

369

nit: do early exit here as well

This revision is now accepted and ready to land.Jun 23 2023, 4:33 AM
hokein updated this revision to Diff 533926.Jun 23 2023, 5:08 AM
hokein marked 2 inline comments as done.

address comments

This revision was landed with ongoing or failed builds.Jun 23 2023, 5:09 AM
This revision was automatically updated to reflect the committed changes.