[PATCH] D70535: [clangd] Define out-of-line qualify return value

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:69
+llvm::Optional<const DeclContext *>
+synthesizeContextForNS(llvm::StringRef TargetNS,
+                       const DeclContext *CurContext) {
----------------
could you add some documentations, e.g. what's the requirement for the input `TargetNS`?

I'm not sure `Synthesize` is clear here, maybe `lookupTargetContext` or `findTargetContext`?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:77
+    while (!CurContext->isTranslationUnit())
+      CurContext = CurContext->getParent();
+  } else {
----------------
nit: maybe use early return?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:117
+  bool HadErrors = false;
+  tooling::Replacements Replacements;
+  findExplicitReferences(FD, [&](ReferenceLoc Ref) {
----------------
could we use a more meaningful name, maybe AddQualifierEdit? Would be nice to have some comments explaining what the following code does.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:149
+        llvm::inconvertibleErrorCode(),
+        "define outline: Failed to compute qualifiers see logs for details.");
+  }
----------------
nit: I think this error message is exposed to users, I'm not sure "see logs for details" is friendly to them.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:153
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+      SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());
----------------
we have similar code in define-inline as well, would be nice to have them in a single place in the long term. probably a FIXME?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:173
 
-  // FIXME: Qualify return type.
   // FIXME: Qualify function name depending on the target context.
 }
----------------
btw, do we need to qualify the function parameters. maybe it is not needed, but would be nice to have some brief comments explaining it.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:116
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
+llvm::Expected<std::string>
----------------
could you also update the comments here, mentioning the function handles the return type qualifiers properly?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1778
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
----------------
can't we merge these into the above `ApplyTest`?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1810
+        })cpp",
+       "a::Foo::Bar foo() { return {}; }\n"},
+      {R"cpp(
----------------
oh, this is very tricky case (I think you meant to test the public members), note that Bar and foo are private member of Foo, we can't move the body out of the class `Foo`, for this case I think we should disallow the tweak.

No need to do it in this patch, but please update the test here (to test public members).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70535