Page MenuHomePhabricator

B32239 clang-tidy should not warn about array to pointer decay on system macros
Needs ReviewPublic

Authored by brenoguim on Mar 19 2017, 12:10 PM.

Details

Summary

Adding a new precondition to avoid warning about array to pointer decay on system macros.

If the ImplicitCastExpr match is casting a "system header symbol" from array to pointer happens inside a system header macro, clang-tidy should not trigger a violation.
I'm calling a "system header symbol" any symbol defined in a system header, or a PredefinedExpr type (like FILE).

Since this is my first submission, there are a few things I couldn't quite figure out:
1 - There is automatic support to switching warn about violations on system header. Maybe I should reuse some of this code?
2 - This check will be supressed even if -system-headers is passed on the command line. I believe, if I reuse the code mentioned in 1, this comes for free. If not possible, I can get the configuration and change my matcher.
I could not figure out how to change the matcher dynamically though. Is there any other way than doing:
if (SystemHeaders) {

Finder->addMatcher(. ...) ;

} else {

Finder->addMatcher(..., extraPrecondition())

}

Diff Detail

Event Timeline

brenoguim created this revision.Mar 19 2017, 12:10 PM

Forgot to add cfe-commits...

aaron.ballman added a subscriber: cfe-commits.
aaron.ballman added inline comments.Mar 20 2017, 3:15 PM
clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp
50

Why not match on ImplicitCastExpr rather than Stmt? Then you can remove the explicit cast below.

55

Likewise here.

59–60

I think this is better-expressed with an isa check: if (isa<PredefinedExpr>(E->getSubExpr())) return true;

63–64

You can combine these into if (const auto *SDR = dyn_cast<>())

brenoguim updated this revision to Diff 92419.Mar 20 2017, 8:02 PM
  • Using the ImplicitCastExpr type on the AST_MATCHER directly to avoid explicit cast.
  • Using isa<> instead of dyn_cast<> to just check for type
  • Move variable declaration into "if" condition
This revision is now accepted and ready to land.Mar 21 2017, 9:56 AM
brenoguim marked 4 inline comments as done.Mar 21 2017, 11:12 AM

Hi @aaron.ballman,

Thanks for the review!

I don't have rights to push this. Who should I contact to push the change in my behalf?

Hi @aaron.ballman,

Thanks for the review!

I don't have rights to push this. Who should I contact to push the change in my behalf?

I'm happy to do so on your behalf (it'll happen later this afternoon unless someone else beats me to it).

I appreciate it!

I've commit in r298421.

aaron.ballman reopened this revision.Mar 21 2017, 6:23 PM

This change was reverted in r298470. The use of the <assert.h> include is a problem because this is not a clang-supplied header file. Also, there's a (good) question about what array decay is happening in the assert(false) test case.

This revision is now accepted and ready to land.Mar 21 2017, 6:23 PM
aaron.ballman requested changes to this revision.Mar 21 2017, 6:23 PM
This revision now requires changes to proceed.Mar 21 2017, 6:23 PM
alexfh edited edge metadata.Mar 22 2017, 6:04 AM

This change was reverted in r298470. The use of the <assert.h> include is a problem because this is not a clang-supplied header file. Also, there's a (good) question about what array decay is happening in the assert(false) test case.

I've made the test script add -nostdinc++ in r298501, which should help catching this kind of an issue earlier.

brenoguim updated this revision to Diff 93062.Mar 25 2017, 9:26 PM
brenoguim edited edge metadata.
  • Removed the "#include <assert.h>" which caused problems in environments without that header
  • Included a directory with -isystem to simulate system headers
  • Added a "macro.h" header with definitions of types, functions and macros to simulate the assert.h header and others.
  • Added checks for behavior in some situations

Some of the checks follow the comment "Not Ok. Should it be ok?" to point the situations in which the check could be considered too strict for reporting such as violation. I would like to get feedback specially on them.

mgehre added inline comments.Mar 27 2017, 1:08 PM
clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp
82

I would say that the check can ignore PredefinedExpr such as __PRETTY_FUNCTION__ by

unless(hasSourceExpression(predefinedExpr ())),
brenoguim updated this revision to Diff 97385.May 1 2017, 8:17 PM
brenoguim marked an inline comment as done.

Never flag predefinedExpr() types.

aaron.ballman added inline comments.May 3 2017, 11:43 AM
test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp
60

I think it's reasonable to diagnose on this due to the decay.

86

Likewise here.

alexfh removed a reviewer: alexfh.May 22 2017, 6:33 AM
aaron.ballman resigned from this revision.Mar 2 2018, 5:56 AM
mgehre resigned from this revision.Jul 27 2018, 12:16 PM