Page MenuHomePhabricator

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

Authored by zinovy.nis on Oct 10 2020, 11:37 AM.

Details

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

Diff Detail

Event Timeline

zinovy.nis created this revision.Oct 10 2020, 11:37 AM
zinovy.nis requested review of this revision.Oct 10 2020, 11:37 AM
Eugene.Zelenko added a project: Restricted Project.
lebedev.ri accepted this revision.Oct 10 2020, 11:53 AM

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

This revision is now accepted and ready to land.Oct 10 2020, 11:53 AM
njames93 added inline comments.Oct 10 2020, 5:34 PM
clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
504–506

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

zinovy.nis marked 3 inline comments as done.Oct 11 2020, 6:25 AM
  • Updated docs on the matcher.
  • Register the new matcher.
zinovy.nis marked an inline comment as done.Oct 11 2020, 7:55 AM

Done. Any other comments/ideas?

lebedev.ri accepted this revision.Oct 11 2020, 7:56 AM

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 accepted this revision.Oct 11 2020, 8:01 AM
JonasToth added a subscriber: 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 added inline comments.Oct 11 2020, 8:02 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4658

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

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.

zinovy.nis marked an inline comment as done.Oct 11 2020, 8:13 AM

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)

zinovy.nis marked an inline comment as done.
JonasToth accepted this revision.Oct 11 2020, 8:31 AM

I am fine now, thank you!

I am fine now, thank you!

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

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

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!