Page MenuHomePhabricator

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

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



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.Wed, Mar 8, 2:28 PM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL requested review of this revision.Wed, Mar 8, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 8, 2:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL updated this revision to Diff 503529.Wed, Mar 8, 2:42 PM

Micro fixes in documentation

Eugene.Zelenko added inline comments.

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


Excessive newline.


Add etc. at the end?



PiotrZSL marked 4 inline comments as done.Wed, Mar 8, 9:27 PM
PiotrZSL updated this revision to Diff 503616.Wed, Mar 8, 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.


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!


Ultra nit: remove extra space


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


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.Sat, Mar 18, 10:47 AM
PiotrZSL marked 3 inline comments as done.

Removed some whitespace & added tests


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.Sat, Mar 18, 11:05 AM

"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


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.Sat, Mar 18, 11:17 AM
PiotrZSL updated this revision to Diff 506310.Sat, Mar 18, 11:18 AM

Change name of isStatic* to isImmutable*

carlosgalvezp accepted this revision.Sat, Mar 25, 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.Sat, Mar 25, 3:37 AM
carlosgalvezp requested changes to this revision.Sat, Mar 25, 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.Sat, Mar 25, 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.Sat, Mar 25, 5:37 AM

Rebase + Shorten documentation

Eugene.Zelenko added inline comments.Sat, Mar 25, 7:04 AM

Please omit Check.



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

Doc update

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