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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:334
+
+    const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+                                                   *QualifiedBody);
----------------
ilya-biryukov wrote:
> 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...
see macro tests for the behavior in such cases.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1332
+  template <>
+  void fo^o<char>(char p) {
+    return;
----------------
ilya-biryukov wrote:
> 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?
yes that's the point, adding a comment.


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:1374
+  )cpp");
+}
+
----------------
ilya-biryukov wrote:
> Do we test the following case anywhere?
> ```
> template <class T> void foo();
> template <> void ^foo<int>() { // should not be available
>   return;
> }
> ```
Yes, there is once check in availability tests for:
```
  EXPECT_UNAVAILABLE(R"cpp(
    template <typename T> void foo();

    template<> void f^oo<int>() {
    })cpp");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66647