hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/SourceCode.h:204
+struct MacroSym {
+ llvm::StringRef Name;
----------------
sammccall wrote:
> 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!)
`DefinedMacro` sounds good to me.
================
Comment at: clang-tools-extra/clangd/SourceCode.h:210
+llvm::Optional<MacroSym> locateMacroAt(SourceLocation Loc,
+ const SourceManager &SM,
+ const LangOptions &LangOpts,
----------------
sammccall wrote:
> you can get the SM and LangOpts from the PP
thanks, remove these two parameters.
================
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(),
----------------
sammccall wrote:
> is the fixme to detect the error, or to support renaming macros?
> (And if we're not going to support them, why not?)
not sure whether we'd support renaming macros eventually, but I believe it is
not prioritized at least for now. I think the macro-handling logic (detect the
error, or support) should live in rename tooling library.
Rephrased the FIXME to make it less confusing.
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
