This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add bugprone-empty-catch check
ClosedPublic

Authored by PiotrZSL on Feb 24 2023, 10:15 AM.

Diff Detail

Event Timeline

PiotrZSL created this revision.Feb 24 2023, 10:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL requested review of this revision.Feb 24 2023, 10:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2023, 10:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
78

Excessive newline.

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
170

Mistake in default value formatting.

PiotrZSL added inline comments.Feb 24 2023, 12:30 PM
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
170

Yy, what mistake ? (empty) not needed, or something else should be used instead of `` ?

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
170

Sorry, I mistook (empty) as part of value. May be just say this in words to avoid such confusions?

PiotrZSL marked 3 inline comments as done.Feb 24 2023, 12:39 PM
PiotrZSL updated this revision to Diff 500274.Feb 24 2023, 12:40 PM

Fix review comments

PiotrZSL updated this revision to Diff 503365.Mar 8 2023, 7:33 AM

Ping, Rebase

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:03 AM
carlosgalvezp added inline comments.Apr 16 2023, 6:02 AM
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
101

Assert that it's not null, or exit early.

105

Nit: I believe a semicolon or dot is more appropriate here

clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
148–154

I'm not sure this option is worth the added complexity - if people want to allow an empty catch with a comment, they might as well just add a NOLINT to suppress the warning? It will be even more clear that it's actually a problem: it's not a regular TODO that doesn't necessarily imply a problem, but rather something that a static analyzer considers a problem.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
12

Fix indentation to 2 chars.

17

Align comments

carlosgalvezp added inline comments.Apr 16 2023, 6:03 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
104

Use 2 spaces indentation.

PiotrZSL marked an inline comment as done.Apr 16 2023, 6:13 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
101

I cannot be null.

105

I can use semicolon

PiotrZSL marked an inline comment as done.Apr 16 2023, 6:13 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
101

It*

PiotrZSL marked 4 inline comments as done.Apr 16 2023, 6:35 AM
PiotrZSL added inline comments.
clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst
148–154

I added this so people could have ability to ignore some issues without putting NOLINT's. This can be also used when developer put some macro into catch block, that would be build dependent, like assert or something.

PiotrZSL updated this revision to Diff 514000.Apr 16 2023, 6:36 AM

Add getCheckTraversalKind, format tests, fix some review comments

xgupta added a subscriber: xgupta.Jul 23 2023, 6:43 AM

Other than these three points everything looks good to me.

clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
72

This can be defined in the header file itself like other checks.

77

This can be defined in the header file itself like other checks.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
4

If TODO is in default then it does not require to be in value here, right?
I tested without that, it gives the warning.

PiotrZSL marked 3 inline comments as done.Jul 23 2023, 6:55 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp
72

Can be, but as this is already virtual function we do not gain anything by putting it into header. vtable will be emited anyway only on this TU.

77

Can be, but as this is already virtual function we do not gain anything by putting it into header. vtable will be emited anyway only on this TU.
And I want to keep it close to registerMatchers, as it impact behavior of that method.

clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp
4

Setting IgnoreCatchWithKeywords manually override default value.

PiotrZSL updated this revision to Diff 543276.Jul 23 2023, 6:56 AM
PiotrZSL marked 3 inline comments as done.

Rebase (release notes)

xgupta accepted this revision.Jul 23 2023, 7:19 AM

LGTM, but we can wait for a couple of before committing in case other reviewers have more review comments.

This revision is now accepted and ready to land.Jul 23 2023, 7:19 AM
This revision was automatically updated to reflect the committed changes.
PiotrZSL reopened this revision.Jul 23 2023, 11:15 AM
This revision is now accepted and ready to land.Jul 23 2023, 11:15 AM
PiotrZSL updated this revision to Diff 543314.Jul 23 2023, 11:18 AM

Add -fexceptions to tests

Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2023, 11:18 AM
This revision was landed with ongoing or failed builds.Jul 23 2023, 11:44 PM
This revision was automatically updated to reflect the committed changes.