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

thakis added a comment. Tests are happy again after 1c66d09 , thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 ___

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

done. sorry for forgetting about this so soon. On Thu, Oct 31, 2019 at 12:58 PM Nico Weber via Phabricator < revi...@reviews.llvm.org> wrote: > thakis added inline comments. > > > > Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1388 >// results in arbitrary fa

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

thakis added a comment. This is failing on Windows: http://45.33.8.238/win/1481/step_7.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 ___ cfe-commits maili

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

thakis added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1388 // results in arbitrary failures as function body becomes NULL. ExtraArgs.push_back("-fno-delayed-template-parsing"); + for (const auto &Case : Cases) You migh

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

merge_guards_bot added a comment. Build result: pass - 59704 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG71aa3f7b7e43: [clangd] Add parameter renaming to define-inline code action (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/

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

kadircet updated this revision to Diff 227233. kadircet marked an inline comment as done. kadircet added a comment. - Address comments - Get rid of raw string literals inside macro calls, to not break stage1 compilers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, many thanks! Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:293 + // Now try to generate edits for all the refs. + for (auto &Ent

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

merge_guards_bot added a comment. Build result: pass - 59704 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

kadircet updated this revision to Diff 227035. kadircet added a comment. - Add comments - Early return when generating edits fails Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: clang-tools-extra/

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

merge_guards_bot added a comment. Build result: pass - 59704 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

kadircet updated this revision to Diff 226909. kadircet marked 2 inline comments as done. kadircet added a comment. - Use Lexer::makeFileCharRange Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: cl

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

kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1517 +template +void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){} + ilya-biryukov wrote: > We fail to rename the parameter in that case, right? > Should the a

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

merge_guards_bot added a comment. Build result: pass - 59704 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1517 +template +void foo(PARAM, TYPE b, TYPE c, TYPE d = BODY(x)){} + We fail to rename the parameter in that case, right? Should the action not be availab

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

kadircet updated this revision to Diff 226868. kadircet marked 3 inline comments as done. kadircet added a comment. - Handle macros in parameter names. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files:

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

ilya-biryukov added a comment. Just a few final NITs from my side. And would also be nice to get tests with macros. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:251 +} +NewName.append(SourceParam->getName()); +ParamToNewName[DestParam->getC

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

merge_guards_bot added a comment. Build result: pass - 59704 tests passed, 0 failed and 762 were skipped. Log files: console-log.txt , CMakeCache.txt

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

kadircet updated this revision to Diff 226849. kadircet added a comment. - Handle unnamed parameters explicitly Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: clang-tools-extra/clangd/refactor/twe

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

merge_guards_bot added a comment. Build result: fail - 59657 tests passed, 2 failed and 805 were skipped. failed: libc++.libcxx/thread/thread_threads/thread_thread_this/sleep_for.pass.cpp failed: LLVM.tools/llvm-ar/mri-utf8.test Log files: cmake-log.txt

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:233 +bool fillTemplateParameterMapping( +const FunctionDecl *Dest, const FunctionDecl *Source, +llvm::DenseMap &ParamToNewName) { NIT: instead of re

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

ilya-biryukov added a comment. D69511 broke the anonymous parameters. Sorry about that, I hope that's for the best in the long run :-) We'll need some code to update this patch. Other than that, I think this patch is good to go! Repository: rG LLVM Github Mo

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

kadircet updated this revision to Diff 226655. kadircet marked 2 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: clang-tools-ex

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

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(Ref.Targets.front()->getCanonicalDecl()); +auto It = Pa

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

merge_guards_bot added a comment. Build result: pass - 59695 tests passed, 0 failed and 763 were skipped. Log files: cmake-log.txt , ninja_check_all-log.txt

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

kadircet updated this revision to Diff 226619. kadircet added a comment. - Don't rename if parameters have the same name Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: clang-tools-extra/clangd/ref

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

merge_guards_bot added a comment. Build result: fail - 59631 tests passed, 1 failed and 805 were skipped. failed: LLVM.tools/llvm-ar/mri-utf8.test Log files: cmake-log.txt , ninja_check_all-log.txt

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

kadircet updated this revision to Diff 226517. kadircet marked 4 inline comments as done. kadircet added a comment. - Use findExplicitReferences for decl traversals. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/

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

ilya-biryukov added a comment. Ah, we're actually trying to rename parameters in the declaration to match the ones in the definition... So the try-catch blocks are not a problem, actually Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ ht

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

ilya-biryukov added a comment. In D68937#1710915 , @kadircet wrote: > I totally agree that the solution you proposed would also work, but don't > think it would be any less code. Since one needs to correlate > parameters between two different declaration

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

kadircet updated this revision to Diff 226389. kadircet added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/ https://reviews.llvm.org/D68937 Files: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp clang-t

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

kuhnel added a comment. Build failed as patch failed to apply: 00:00:06.098 Checking patch clang-tools-extra/trunk/clangd/FindTarget.h... 00:00:06.100 error: clang-tools-extra/trunk/clangd/FindTarget.h: does not exist in index 00:00:06.100 Checking patch clang-tools-extra/trunk/clangd/Find

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

kadircet updated this revision to Diff 225209. kadircet marked an inline comment as done. kadircet added a comment. - Accept StringRef for Source in rewriteParameterName instead of a NamedDecl Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68937/new/

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

kadircet added a comment. In D68937#1710634 , @ilya-biryukov wrote: > An alternative approach I'm thinking of: > After D68977 lands, we could try using > `findExplicitReferences` to produce all of these edits: > > 1. we

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

ilya-biryukov added a comment. An alternative approach I'm thinking of: After D68977 lands, we could try using `findExplicitReferences` to produce all of these edits: 1. we collect locations of all references and declaration of relevant parameters and template

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

kadircet created this revision. kadircet added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay. Herald added a project: clang. When moving a function definition to declaration location we also need to handle renaming of the both function and