This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add performance-enum-size check
ClosedPublic

Authored by PiotrZSL on Feb 15 2023, 1:57 PM.

Details

Summary

Finds enum type definitions that could use smaller integral type as a base.

Diff Detail

Event Timeline

PiotrZSL created this revision.Feb 15 2023, 1:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL updated this revision to Diff 497787.Feb 15 2023, 2:03 PM

Removed change in list.rst that is related to cppcoreguidelines-avoid-capture-default-when-capturing-this.
Looks like that check have fixes but didnt mark fixes in that file.

PiotrZSL updated this revision to Diff 497804.Feb 15 2023, 2:23 PM

Fix typo in documentation.

PiotrZSL updated this revision to Diff 497826.Feb 15 2023, 3:32 PM

Fix tests on windows (they use int as base for enums, when on linux unsigned int is used)

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
99

Please do not use auto unless type is explicitly spelled in same function or iterator.

100

I don't think that literal boolean operators are part of LLVM coding guidelines.

103

Ditto.

120

Ditto.

PiotrZSL marked 4 inline comments as done.Feb 15 2023, 10:40 PM
PiotrZSL updated this revision to Diff 497887.Feb 15 2023, 10:40 PM

Fix review comments

PiotrZSL published this revision for review.Feb 15 2023, 11:22 PM

Ready for review

Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2023, 11:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL updated this revision to Diff 498471.Feb 17 2023, 11:52 AM

Updated documentation.

PiotrZSL updated this revision to Diff 498472.Feb 17 2023, 11:54 AM

Typo in doc

PiotrZSL planned changes to this revision.Mar 5 2023, 1:21 PM

Option need to be changed from Regex to List

PiotrZSL updated this revision to Diff 502790.Mar 6 2023, 1:49 PM

Ping, Rebase, Changed Regexp option into List

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:06 AM
PiotrZSL updated this revision to Diff 518339.Apr 30 2023, 11:21 AM

Ping, Rebase

unterumarmung added inline comments.
clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
128–129

I find the enum's type derivation message to be a bit unintuitive. It would slightly improve the user experience if the error message provided clearer information, like stating "enum %0 has a base type of %1..." or "the base type of enum %0 appears excessive for its value set...". However, please remember that these are merely personal thoughts, and as a non-contributor, my suggestions are not obligatory.

clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
63

Why not?

PiotrZSL marked an inline comment as done.Jul 5 2023, 8:37 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/performance/EnumSizeCheck.cpp
128–129

Hmm, ok, I think I can change it to utilize a "base type" instead of "derive" and something like "appears excessive for its value set.".

clang-tools-extra/docs/clang-tidy/checks/performance/enum-size.rst
63

Problem is mainly with forward declarations, and a fact that some of these changes may be unnecessary from a domain point of view. I would prefer users to change enum sizes on their own risk. And I didn't wanted to add this at the beginning, in future maybe.

xgupta added a subscriber: xgupta.Jul 22 2023, 7:37 AM

This LGTM, but can't officially accept the patch so please wait for another reviewer to accept it.

PiotrZSL marked an inline comment as done.

@Eugene.Zelenko I cannot review/accept my own patch :(.

@Eugene.Zelenko I cannot review/accept my own patch :(.

I'm sorry for noise.

PiotrZSL updated this revision to Diff 543196.Jul 22 2023, 8:42 AM
PiotrZSL marked an inline comment as done.

Rebase + Update diagnostic message

xgupta accepted this revision.Jul 25 2023, 9:59 AM

LGTM, thanks.

This revision is now accepted and ready to land.Jul 25 2023, 9:59 AM
This revision was automatically updated to reflect the committed changes.