[clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[zinovy.nis]

Summary

Assert fires on weak refs like

static void dont_crash_on_weak() attribute((weakref("__dont_crash_on_weak")));

Bug: https://bugs.llvm.org/show_bug.cgi?id=47779

[lebedev.ri]

Looks good to me, @aaron.ballman ?

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity.cpp

1017

1018

Please do add a newline

clang/include/clang/ASTMatchers/ASTMatchers.h

4650

You also need to regenerate the html docs then

[njames93]

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

504

Not strictly necessary but that allOfcan be removed. All top level matchers are Implicit allOf matchers.

[zinovy.nis]

• Updated docs on the matcher.
• Register the new matcher.

[zinovy.nis]

Done. Any other comments/ideas?

[lebedev.ri]

Since this is touching more than just a clang-tidy check
i still want someone else to sign off,
but this looks ok to me in principle.
Thanks.

[JonasToth]

LGTM.
Short reminder to please use full context patches. Its not an issue right now, but review sometimes just needs the context to understand the changes :)

[JonasToth]

clang/include/clang/ASTMatchers/ASTMatchers.h

4658

Short oversight. Please hightlight the code and matcher to make a differentation in the docs.

[zinovy.nis]

Full diff.

[zinovy.nis]

In D89194#2323868, @JonasToth wrote:

LGTM.
Short reminder to please use full context patches. Its not an issue right now, but review sometimes just needs the context to understand the changes :)

Sorry. Done.

clang/include/clang/ASTMatchers/ASTMatchers.h

4658

Sorry, I did not fully get what you mean as I just cloned isDefaulted section.

[JonasToth]

The context thingie was not that important here, but still thanks :)

clang/include/clang/ASTMatchers/ASTMatchers.h

4658

Its not consistent in the docs, but e.g.
typedefNameDecl() matches "typedef int X" and "using Y = int" is just more readable in the slightly formatted docs.

my preference would be "functionDecl(isWeak()) matches the weak declaration "foo", but not "bar".
(If you do the chance don't forget to recreate the HTML)

[JonasToth]

I am fine now, thank you!

[zinovy.nis]

In D89194#2323917, @JonasToth wrote:

I am fine now, thank you!

Thanks!
Is this patch ready for landing? Or should I wait for a few more approvals?

[JonasToth]

You can land it. Worst case would be post-commit comments, but this is not a complicated patch and should be fine!

[lebedev.ri]

In D89194#2323920, @JonasToth wrote:

You can land it. Worst case would be post-commit comments, but this is not a complicated patch and should be fine!

+1

@JonasToth Thanks!

clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp

 
 void FunctionCognitiveComplexityCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
-      functionDecl(
-          allOf(isDefinition(), unless(anyOf(isDefaulted(), isDeleted(),
-                                             isImplicit(), isInstantiated()))))
+      functionDecl(isDefinition(),
+                   unless(anyOf(isDefaulted(), isDeleted(), isImplicit(),
+                                isInstantiated(), isWeak())))
           .bind("func"),
       this);
 }

clang-tools-extra/test/clang-tidy/checkers/readability-function-cognitive-complexity.cpp

 
   templatedFunction<void*>();
 }
+
+static void pr47779_dont_crash_on_weak() __attribute__((__weakref__("__pr47779_dont_crash_on_weak")));

clang/docs/LibASTMatchersReference.html

 </pre></td></tr>
 
 
+<tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('isWeak0')"><a name="isWeak0Anchor">isWeak</a></td><td></td></tr>
+<tr><td colspan="4" class="doc" id="isWeak0"><pre>Matches weak function declarations.
+
+Given:
+  void foo() __attribute__((__weakref__("__foo")));
+  void bar();
+functionDecl(isWeak())
+  matches the weak declaration foo, but not bar.
+</pre></td></tr>
+
+
 <tr><td>Matcher&lt;<a href="https://clang.llvm.org/doxygen/classclang_1_1FunctionDecl.html">FunctionDecl</a>&gt;</td><td class="name" onclick="toggle('parameterCountIs0')"><a name="parameterCountIs0Anchor">parameterCountIs</a></td><td>unsigned N</td></tr>
 <tr><td colspan="4" class="doc" id="parameterCountIs0"><pre>Matches FunctionDecls and FunctionProtoTypes that have a
 specific parameter count.

clang/include/clang/ASTMatchers/ASTMatchers.h

   return Node.isDefaulted();
 }
 
+/// Matches weak function declarations.
+///
+/// Given:
+/// \code
+///   void foo() __attribute__((__weakref__("__foo")));
+///   void bar();
+/// \endcode
+/// functionDecl(isWeak())
+///   matches the weak declaration foo, but not bar.
+AST_MATCHER(FunctionDecl, isWeak) { return Node.isWeak(); }
+
 /// Matches functions that have a dynamic exception specification.
 ///
 /// Given:

clang/lib/ASTMatchers/Dynamic/Registry.cpp

   REGISTER_MATCHER(isVirtual);
   REGISTER_MATCHER(isVirtualAsWritten);
   REGISTER_MATCHER(isVolatileQualified);
+  REGISTER_MATCHER(isWeak);
   REGISTER_MATCHER(isWritten);
   REGISTER_MATCHER(lValueReferenceType);
   REGISTER_MATCHER(labelDecl);