[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

This revision was automatically updated to reflect the committed changes. Closed by commit rGdfd6374c784f: [clangd] DefineInline action apply logic with fully qualified names (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 226388. kadircet marked 2 inline comments as done. kadircet added a comment. - s/SemiColon/Semicolon/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-to

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov accepted this revision. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:339 + +const tooling::Replacement

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334 + +const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1, + *QualifiedBody); ilya-biryukov w

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 226230. kadircet marked 15 inline comments as done. kadircet added a comment. - Improve macro handling Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-t

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. A few last NITs and one important comment about handling the case when function definition come from macro expansions Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323 + +

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208 + + if (HadErrors) { +return llvm::createStringError( kadircet wrote: > ilya-biryukov wrote: > > NI

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79 + Token CurToken; + while (!CurToken.is(tok::semi)) { +if (RawLexer.LexFromRawLexer(CurToken)) kadircet wrote: > ilya-biryukov wrote: > > What are

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79 + Token CurToken; + while (!CurToken.is(tok::semi)) { +if (RawLexer.LexFromRawLexer(CurToken)) ilya-biryukov wrote: > What are the tokens we expect to s

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. Mostly comments about tests, the implementation itself LG. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68 +llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) { + const SourceManager &SM = FD->getASTContext().ge

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226 +llvm::Expected +renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) { + const SourceManager &SM = Dest->getASTContext().getSourceManager(); -

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 224824. kadircet added a comment. - Move parameter renaming logic to a separate patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra/clang

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196 + +std::string Qualifier = printNamespaceScope(*ND->getDeclContext()); +if (auto Err = Replacements.add( ilya-biryukov wrote: > You would want to use

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68 +// Lexes from end of \p FD until it finds a semicolon. +llvm::Optional getSemiColonForDecl(const FunctionDecl *FD) { "Lexes" is probably a not very re

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 222892. kadircet marked 5 inline comments as done. kadircet added a comment. - Update test helper to take an out parameter Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D666

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 222562. kadircet added a comment. - Fix getSemiColon to handle semicolons at the end of file Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. In D66647#1686021 , @kadircet wrote: > So currently AST doesn't store any information regarding template parameter > locations except the deepest one. > Therefore I've changed the availability to discard any methods inside

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 222180. kadircet added a comment. - Add renaming of template and function parameters Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra/clangd/

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added a comment. In D66647#1684046 , @ilya-biryukov wrote: > We also need to rename parameters sometimes, right? > > // Sometimes we need to rename parameters. > void usages(int decl_param, int); > > void usages(int def_param, int now_nam

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:77 // - if apply() returns an error, returns "fail: " - std::string apply(llvm::StringRef MarkedCode) const; + std::string apply(llvm::StringRef MarkedCode); ---

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added a comment. In D66647#1685554 , @ilya-biryukov wrote: > Are we planning to fix this right away or should we keep this indefinitely? > If latter, I believe we should think about some heuristics to avoid > qualification in the short term ins

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. In D66647#1685506 , @kadircet wrote: > I actually have a fixme saying: > > // FIXME: Instead of fully qualifying we should try deducing visible scopes > at > // target location and generate minimal edits. > > > Elaborati

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

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

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet added a comment. In D66647#1684012 , @ilya-biryukov wrote: > It's ok to leave this out of the initial change, but could we describe our > strategy to tackle this somewhere in the comments - how we want to fix this > and when. I actually have

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

hokein added a comment. In D66647#1684046 , @ilya-biryukov wrote: > We also need to rename parameters sometimes, right? > > // Sometimes we need to rename parameters. > void usages(int decl_param, int); > > void usages(int def_param, int now_named

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. We also need to rename parameters sometimes, right? // Sometimes we need to rename parameters. void usages(int decl_param, int); void usages(int def_param, int now_named) { llvm::errs() << def_param + now_named; } // And template parameters! (the

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added a comment. Currently we add too many qualifiers in some cases, e.g. here's a common pattern in clangd: // foo.h #include "Decl.h" namespace clang::clangd { std::string printName(Decl*D); } // foo.cpp #include "foo.h" namespace clang::clangd { std::string p

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 221913. kadircet added a comment. - Mark tweak as visible Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra/clangd/refactor/tweaks/DefineInlin

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet updated this revision to Diff 221912. kadircet added a comment. - Rebase and update testhelpers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 Files: clang-tools-extra/clangd/refactor/tweaks/Def

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:158 + // i.e: a::Bar> instead of a::Bar> + printTemplateArgumentList(OS, TSTL.getTypePtr()->template_arguments(), +TD->getASTConte

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

kadircet created this revision. kadircet added reviewers: sammccall, ilya-biryukov. Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay. Herald added a project: clang. kadircet added a parent revision: D65433: [clangd] DefineInline action availability checks. Initial version of Defi