[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly comments about tests, the implementation itself LG.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:68
 
+llvm::Optional<SourceLocation> getSemiColonForDecl(const FunctionDecl *FD) {
+  const SourceManager &SM = FD->getASTContext().getSourceManager();
----------------
NIT: s/SemiColon/Semicolon


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:79
+  Token CurToken;
+  while (!CurToken.is(tok::semi)) {
+    if (RawLexer.LexFromRawLexer(CurToken))
----------------
What are the tokens we expect to see before the semicolon here?
It feels safer to just skip comments and whitespace and check the next token is a semicolon.

Any reason to have an actual loop here?


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:181
+      if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
+        elog("Targets from multiple contexts: {0}, {1}",
+             printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND));
----------------
NIT: could we mention the tweak name here? to make sure it's easy to detect those lines in the logs:
`define inline: targets from multiple contexts`


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:208
+
+  if (HadErrors) {
+    return llvm::createStringError(
----------------
NIT: braces are redundant


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:320
   Expected<Effect> apply(const Selection &Sel) override {
-    return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                   "Not implemented yet");
+    const ASTContext &AST = Sel.AST.getASTContext();
+    const SourceManager &SM = AST.getSourceManager();
----------------
NIT: use auto 


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:325
+    if (!SemiColon) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
----------------
NIT: braces are redundant


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.cpp:85
+std::string TweakTest::apply(llvm::StringRef MarkedCode,
+                             llvm::StringMap<std::string> *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
----------------
Could you document `EditedFiles` contains all other edited files and the original `std::string` only contains results for the main file?
Would also be useful to document what is the key in the StringMap: absolute/relative paths or URI?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:905
+  EXPECT_EQ(apply(R"cpp(
+  void foo()    /*Comment -_-*/  ;
+
----------------
Why have a comment in this test?
If we need to test we handle comments before the semicolon, could we move to a separate focused test?
If we want to test something else, let's leave out the comment. The test is hard enough to read on its own


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:922
+
+    template <typename T> class Bar {};
+    Bar<int> z;
----------------
Template declarations inside function bodies are not allowed in C++.
Are we getting compiler errors here and not actually testing anything?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:961
+
+  void foo()    /*Comment -_-*/  ;
+
----------------
Same here: we don't need the comment here.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:964
+  void f^oo() {
+    using namespace a;
+    bar<Bar<int>>.bar();
----------------
Either remove `using namespace a` from the function body or put a FIXME this shouldn't actually qualify?
I believe the first option is what we want, if we're trying to check template arguments are getting transformed properly


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:989
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
----------------
Maybe move this matcher to the start of the file?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1029
+  EXPECT_THAT(EditedFiles,
+              testing::ElementsAre(FileWithContents(testPath("a.h"),
+                                                    R"cpp(
----------------
We prefer to have `using ::testing::ElementsAre` at the start of the file and not qualify the usage everywhere in our tests.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1076
+  template <>
+  void foo<int>(int p);
+
----------------
This is really uncommon, I'm not even sure we should test this.
The other case is much more common and interesting:

```
template <class T>
void foo(T p);

template <>
void foo<int>(int p) { /* do something */ }

```

Could we add a test that we don't suggest this action in that case?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1167
+  }
+  using namespace a;
+  using namespace b;
----------------
Can we test with a single namespace?
Having 3 layers of nested namespace in every test makes reading them very complicated.

Instead, we could validate with separate tests:
- that the qualification logic works with multiple nested namespaces,
- that various kinds of references are properly qualified with a single namespace

I believe that would keep both tests more focused


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1215
+    namespace b {
+    class Foo{};
+    namespace c {
----------------
Could you indent here and in other tests? it's hard to follow the fact that `Foo` is inside `b` without indentation.
Same for other tests.

Alternatively, put on one line if there's just a single declaration inside the namespace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647