[PATCH] D65433: [clangd] DefineInline action availability checks

[Haojian Wu via Phabricator via cfe-commits]

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