hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:65
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+ const ast_type_traits::DynTypedNode &AstNode = SelNode->ASTNode;
+ if (const FunctionDecl *FD = AstNode.get<FunctionDecl>())
----------------
nit: maybe use `auto`?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:78
+// function decl. Skips local symbols.
+llvm::DenseSet<const Decl *> getNonLocalDeclRefs(const FunctionDecl *FD,
+ ParsedAST &AST) {
----------------
thinking this a bit more, the function is non-trivial, we probably want unit
test for it.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:105
+ Opts.SystemSymbolFilter =
+ index::IndexingOptions::SystemSymbolFilterKind::None;
+
----------------
IIUC, I believe this function relies on the flag "Opts.IndexFunctionLocals"?
could we spell it explicitly in the code even its default value `false` fits
our use case.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:113
+// Checks the decls mentioned in Source are visible in the context of Target.
+// Achives that by performing lookups in Target's DeclContext.
+// Unfortunately these results need to be post-filtered since the AST we get is
----------------
hmm, looks like we don't use TargetContext in the implemenation at all. Is the
comment out-of-date now?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:141
+ // same class.
+ if (!SM.isBeforeInTranslationUnit(D->getLocation(),
+ Target->getLocation())) {
----------------
we are distinguishing two cases here: normal functions and the class methods.
Could we make the code handle these two cases separately? e.g.
```
if (Class) {
// check whether they are in the same class;
} else { // handle normal function
// call isBeforeInTranslation
}
```
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:160
+// usually be the first declaration in current translation unit with the
+// exception of template specialization. For those we return the previous
+// declaration instead of the first one, since it will be template decl itself
----------------
function template is tricky, I may be not aware of the context, what's our plan
for supporting it? could you give an concrete example? it seems that the
current unit test doesn't cover it.
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:172
+
+/// Moves definition of a function to its declaration location.
+/// Before:
----------------
I believe it also works for class methods?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:183
+/// a.h:
+/// void foo() { return ; }
+///
----------------
now we get a potential ODR violation in this example, maybe choose a different
example?
================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:224
+
+ Expected<Effect> apply(const Selection &Sel) override {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
----------------
The tweak is under development, do we need to hide it until it is completed?
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:25
#include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/Tooling/Core/Replacement.h"
----------------
the header seems irrelevant to me.
================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:709
+ EXPECT_UNAVAILABLE(R"cpp(
+ #include "a.h"
+ void bar();
----------------
The `a.h` seems to be non-existed unless you explicitly set it via
`ExtraFiles["a.h"]`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65433/new/
https://reviews.llvm.org/D65433
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
