[PATCH] D68937: [clangd] Add parameter renaming to define-inline code action

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:278
+        const auto *Target =
+            llvm::dyn_cast<NamedDecl>(Ref.Targets.front()->getCanonicalDecl());
+        auto It = ParamToNewName.find(Target);
----------------
why not `llvm::cast`? We'd rather fail early (during the cast) than postpone it until we dereference `Target` a few lines further


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:285
+          return;
+        const size_t OldNameLen = Target->getName().size();
+        // If decl is unnamed in destination we pad the new name to avoid gluing
----------------
Could we measure the token length instead?
There are various bizzare cases when the source text is different and we definitely don't want to accidentally crash on those, e.g.
```
int a = N\
A\
ME;
```


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:295
+          RenamingErrors =
+              llvm::joinErrors(std::move(RenamingErrors), std::move(Err));
+        }
----------------
Wow, TIL I learned something new. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68937/new/

https://reviews.llvm.org/D68937