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

[Sam McCall via Phabricator via cfe-commits]

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