This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add readability-avoid-unconditional-preprocessor-if check
ClosedPublic

Authored by PiotrZSL on Mar 8 2023, 2:28 PM.

Details

Summary

Check flags always enabled or disabled code blocks in preprocessor '#if'
conditions, such as '#if 0' and '#if 1' etc.

Diff Detail

Event Timeline

PiotrZSL created this revision.Mar 8 2023, 2:28 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL requested review of this revision.Mar 8 2023, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 2:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL updated this revision to Diff 503529.Mar 8 2023, 2:42 PM

Micro fixes in documentation

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp
30

Please don't use auto unless type is spelled explicitly in same statement or iterator.

44

Excessive newline.

clang-tools-extra/docs/ReleaseNotes.rst
140

Add etc. at the end?

clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst
8

Ditto.

PiotrZSL marked 4 inline comments as done.Mar 8 2023, 9:27 PM
PiotrZSL updated this revision to Diff 503616.Mar 8 2023, 9:28 PM
PiotrZSL edited the summary of this revision. (Show Details)

Review comments fixes.

There are 186 findings from this check in llvm repository, no false-positives.

Example:

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:2071:2: warning: preprocessor condition is always 'true', consider removing condition but leaving its contents [readability-avoid-unconditional-preprocessor-if]
clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp:400:2: warning: preprocessor condition is always 'false', consider removing both the condition and its contents [readability-avoid-unconditional-preprocessor-if]

Looks good, minor comments!

clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.h
17

Ultra nit: remove extra space

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp
18–32

For completeness, would it make sense to add the same test but with the "is always 'false'" case?

68–70

Add case for if 10 < DDD ?

Also, add test case for comparing 2 macros? If people follow the "no magic numbers" policy they'll likely have defines for both sides of the comparison.

PiotrZSL updated this revision to Diff 506304.Mar 18 2023, 10:47 AM
PiotrZSL marked 3 inline comments as done.

Removed some whitespace & added tests

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp
68–70

I don't support currently macros, would need to check if they defined unconditionally in same file, and I didn't want to mess with this.

carlosgalvezp added inline comments.Mar 18 2023, 11:05 AM
clang-tools-extra/clang-tidy/readability/AvoidUnconditionalPreprocessorIfCheck.cpp
41

"static" is an overloaded C++ keyword which can lead to confusion in this context (you don't intend to detect whether a token is the static keyword). Would it be possible to give this a different name? Same for isStaticToken

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-unconditional-preprocessor-if.cpp
68–70

Yes I didn't mean "function-like macros", just a plain define like you just added. Great, thanks!

PiotrZSL marked an inline comment as done.Mar 18 2023, 11:17 AM
PiotrZSL updated this revision to Diff 506310.Mar 18 2023, 11:18 AM

Change name of isStatic* to isImmutable*

carlosgalvezp accepted this revision.Mar 25 2023, 3:37 AM

LGTM thank you! Really useful check! I'm not very expert in the PP callbacks so I can't give much feedback, feel free to wait for other more expert reviewers if you want.

This revision is now accepted and ready to land.Mar 25 2023, 3:37 AM
carlosgalvezp requested changes to this revision.Mar 25 2023, 3:38 AM

I noticed the pre-merge checks are red, however, would be good to get them fixed.

This revision now requires changes to proceed.Mar 25 2023, 3:38 AM

@carlosgalvezp Do you want me to shorten documentation ? Or we leave it like it is ?

PiotrZSL updated this revision to Diff 508302.Mar 25 2023, 5:37 AM

Rebase + Shorten documentation

Eugene.Zelenko added inline comments.Mar 25 2023, 7:04 AM
clang-tools-extra/docs/ReleaseNotes.rst
139

Please omit Check.

clang-tools-extra/docs/clang-tidy/checks/readability/avoid-unconditional-preprocessor-if.rst
7

Ditto.

PiotrZSL updated this revision to Diff 508320.Mar 25 2023, 9:41 AM

Doc update

PiotrZSL marked 2 inline comments as done.Mar 25 2023, 9:48 AM

@carlosgalvezp Do you want me to shorten documentation ? Or we leave it like it is ?

It's fine for me either way!

carlosgalvezp accepted this revision.Mar 26 2023, 6:13 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 26 2023, 6:13 AM