[clang-tools-extra] [clangd] DefineInline: only trigger on the declaration that has the body (PR #227534)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clangd

Author: Mrgoudan

<details>
<summary>Changes</summary>

The "Move function body to declaration" tweak (DefineInline) is offered on a
redeclaration that follows the definition, and then fails when applied.

### Reproduction

```
printf 'void h(void) {}\nvoid h(void);\n' &gt; /tmp/p3.c
clangd --check=/tmp/p3.c 2&gt;&amp;1 | grep -E 'DefineInline|All checks'
```

Before (also reproduces with clangd 17.0.6 and 19.1.7):

```
tweak: DefineInline ==&gt; FAIL: Couldn't find semicolon for target declaration.
All checks completed, 1 errors
```

After:

```
All checks completed, 0 errors
```

### Cause

`DefineInline::prepare` guards on `Source-&gt;hasBody()`, which is true for any
declaration of a function that is defined anywhere in its redeclaration chain.
Selecting a redeclaration that comes after the definition therefore passes the
guard. `findTarget()` returns the canonical declaration, which in that order is
the definition itself, so `Target != Source` and `prepare` succeeds. `apply`
then looks for a `;` after the definition's body and fails. The fix is to use
`doesThisDeclarationHaveABody()`, which is true only for the declaration that
carries the body. The existing test only covered a prototype before the
definition, where `Target == Source` masked the wrong guard; this adds the
redeclaration-after-definition case.

This change was prepared with the assistance of an AI tool (Claude); I have
reviewed and tested it.


---
Full diff: https://github.com/llvm/llvm-project/pull/227534.diff


2 Files Affected:

- (modified) clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp (+1-1) 
- (modified) clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp 
(+8) 


``````````diff
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp 
b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
index c9704492bf1cd..2661e71d25651 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
@@ -400,7 +400,7 @@ class DefineInline : public Tweak {
     if (!SelNode)
       return false;
     Source = getSelectedFunction(SelNode);
-    if (!Source || !Source->hasBody())
+    if (!Source || !Source->doesThisDeclarationHaveABody())
       return false;
     // Only the last level of template parameter locations are not kept in AST,
     // so if we are inlining a method that is in a templated class, there is no
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp 
b/clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
index 5ec12396ae927..97f82fbb55151 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineInlineTests.cpp
@@ -48,6 +48,14 @@ TEST_F(DefineInlineTest, TriggersOnFunctionDecl) {
   // Definition with no body.
   class Bar { Bar() = def^ault; };
   )cpp");
+
+  EXPECT_UNAVAILABLE(R"cpp(
+  // Redeclaration after the definition.
+  void foo() {
+    return;
+  }
+  vo^id f^oo();
+  )cpp");
 }
 
 TEST_F(DefineInlineTest, NoForwardDecl) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/227534
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits