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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

A few last NITs and one important comment about handling the case when function definition come from macro expansions



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:323
+
+    auto SemiColon = getSemicolonForDecl(Target);
+    if (!SemiColon) {
----------------
NIT: s/SemiColon/Semicolon


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334
+
+    const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+                                                   *QualifiedBody);
----------------
Could we add a test when the semicolon comes from a macro expansion? I believe the action won't be available in that case, which is ok. Just to make sure we cover this corner-case.

```
#define SEMI ;
void foo() SEMI
```

Also interesting cases:
- source function is in a macro expansion
- target function is in a macro expansion.

I believe they might catch a few bugs here, as we seem to assume all locations are file locations...


================
Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:73
+  //    for the main file.
+  //    Populates \p EditedFiles if there were changes to other files whenever
+  //    it is non-null. It is a mapping from absolute path of the edited file to
----------------
Hm, maybe even call `ADD_FAILURE()` when there are changes to other files and tests pass `null` to `EditedFiles`?
Likely an error in the test.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1226
+    void foo(){
+    return;
+  }
----------------
Argh... Without `clang-format` that looks SO weird :-)


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1266
+  template <typename T>
+  void foo();
+
----------------
Maybe put it before `using  namespace a;`?
To make sure the test does not need to change if we actually support not qualifying when not neccessary


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+  template <>
+  void fo^o<char>(char p) {
+    return;
----------------
How is this test different from the previous one?
Is it just trying to make sure we don't always move to the first function body?


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+  )cpp");
+}
+
----------------
Do we test the following case anywhere?
```
template <class T> void foo();
template <> void ^foo<int>() { // should not be available
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647