sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:661
+ Preprocessor &PP) {
+ // Also handle possible macro at the searched location.
+ Token Result;
----------------
"also" no longer makes sense here
================
Comment at: clang-tools-extra/clangd/SourceCode.h:204
+struct MacroSym {
+ llvm::StringRef Name;
----------------
not sure about "sym" - it's an abbreviation and not very descriptive.
`MacroDefinition` is really the best name, but taken :-(
Maybe `DefinedMacro` or `ResolvedMacro`?
================
Comment at: clang-tools-extra/clangd/SourceCode.h:204
+struct MacroSym {
+ llvm::StringRef Name;
----------------
sammccall wrote:
> not sure about "sym" - it's an abbreviation and not very descriptive.
> `MacroDefinition` is really the best name, but taken :-(
>
> Maybe `DefinedMacro` or `ResolvedMacro`?
(thanks for moving this here, definitely makes sense!)
================
Comment at: clang-tools-extra/clangd/SourceCode.h:209
+// Gets the macro at a specified \p Loc.
+llvm::Optional<MacroSym> locateMacroAt(SourceLocation Loc,
+ const SourceManager &SM,
----------------
Can we add tests for this now that it's a public API?
================
Comment at: clang-tools-extra/clangd/SourceCode.h:210
+llvm::Optional<MacroSym> locateMacroAt(SourceLocation Loc,
+ const SourceManager &SM,
+ const LangOptions &LangOpts,
----------------
you can get the SM and LangOpts from the PP
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:167
AST, Pos, AST.getSourceManager().getMainFileID());
+ // FIXME: this should be moved to rename tooling library?
+ if (locateMacroAt(SourceLocationBeg, ASTCtx.getSourceManager(),
----------------
is the fixme to detect the error, or to support renaming macros?
(And if we're not going to support them, why not?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63922/new/
https://reviews.llvm.org/D63922
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
