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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
