[PATCH] D63922: [clangd] Show better message when we rename macros.

[Haojian Wu via Phabricator via cfe-commits]

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