[PATCH] D122698: [clangd] Add support to extract method for ExtractFunction Tweak

[Fabio R. Sluzala via Phabricator via cfe-commits]

FabioRS added a comment.

In D122698#3418018 <https://reviews.llvm.org/D122698#3418018>, @sammccall wrote:

> Thanks! I'll look at the changes in the morning, just wanted to mention one thing
>
> In D122698#3417946 <https://reviews.llvm.org/D122698#3417946>, @FabioRS wrote:
>
>> I can not run the test the consteval, so it is not in the diff.
>>
>>   TestTU failed to build (suppress with /*error-ok*/): 
>>   [1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, fixes: {change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}
>
> It's a c++20 feature, so you'll need to add something like `ExtraArgs.push_back("-std=c++20");`.
>
> Though if there's no real difference between handling constexpr vs consteval, up to you whether it's worth a separate test.

Thank you!
I think it is interesting to have the test to keep the consteval branch covered. I think maybe there will be more things to change, so I can send the test case for consteval tomorrow with the others changes.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:455
+  return std::string(llvm::formatv("{0}{1} {2}({3}){4}", renderAttributes(),
+                                   printType(ReturnType, *EnclosingFuncContext),
+                                   Name, renderParametersForDefinition(),
----------------
sammccall wrote:
> For out-of-line defined methods, this is the wrong DC, which may lead to the type being incorrectly qualified. The DC should rather be the enclosing class.
> 
> Similarly, renderParametersForDefinition uses EnclosingFuncContext too - instead it should take the EnclosingFuncContext as a parameter. (And be renamed `renderParametersForDeclaration`, since it's no longer just for definitions)
It is only for out-of-line methods? I put this verification in the getEnclosing() of the new diff.



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

https://reviews.llvm.org/D122698