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<SourceLocation> getSemiColonForDecl(const FunctionDecl *FD) { ---------------- "Lexes" is probably a not very relevant implementation detail. I'd say this function probably doesn't need a comment, it's clear what it does from its name. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:169 + tooling::Replacements Replacements; + std::string Failure; + findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) { ---------------- NIT: Maybe store `Error` directly? It would communicate the intent of the variable more clearly. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:175 + // qualifier. + if (Ref.Qualifier.hasQualifier()) + return; ---------------- NIT: IIUC, this could be simply `if (Ref.Qualifier)` ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:182 + + // All Targets should be in the same nested name scope, so we can safely + // chose any one of them. ---------------- As discussed before, `Targets` can actually come from different scopes, e.g. from ADL. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183 + // All Targets should be in the same nested name scope, so we can safely + // chose any one of them. + const NamedDecl *ND = Ref.Targets.front(); ---------------- It just occurred to me that this is a really bad idea. I **think** the order of items in `Targets` is non-deterministic. Could we instead try to check if all scopes are the same and only qualify in that case? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:196 + + std::string Qualifier = printNamespaceScope(*ND->getDeclContext()); + if (auto Err = Replacements.add( ---------------- You would want to use `ND->printNestedNameSpecifier()` instead to avoid printing inline namespaces: ``` namespace std { inline namespace v1 { struct vector {}; }} ``` ^-- I believe the current code would print `std::v1::` for `vector` and we want `std::`. Could you add this example to tests too? ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:199 + tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) { + Failure = llvm::formatv("Failed to add qualifier: {0}", + llvm::fmt_consume(std::move(Err))); ---------------- Maybe return a generic error in the end instead of the last error? Something like `createStringError(..., "Failed to compute qualifiers, see errors in the logs for more details")`. Should be a better UX for anyone who tries to explore what went wrong. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:223 + +/// Generates Replacements for changing parameter names in \p Dest to be the +/// same as in \p Source. ---------------- NIT: mention that both function parameters and template parameters are updated here. ================ Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:226 +llvm::Expected<tooling::Replacements> +renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) { + const SourceManager &SM = Dest->getASTContext().getSourceManager(); ---------------- This does not rename any references to those parameters, right? E.g. ``` template <class T> void foo(T x); template <class U> void foo(U x) {} ``` would turn into ``` template <class U> void foo(T x); ``` right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66647/new/ https://reviews.llvm.org/D66647 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits