This is an archive of the discontinued LLVM Phabricator instance.

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

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
fiesh added a subscriber: fiesh.Oct 16 2019, 2:00 PM

Ping! Am I correct in that basically everything's done here and this has been lingering ever since? It would be great if the change could make it.

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

fiesh added a comment.Nov 25 2019, 1:45 AM

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

What are the options in case the original author has abandonded this? Can anybody re-create the MR under a new license with the commit message fixed, or is that an insurmountable license issue?

The description only says what the patch does, not why this is the desired behavior.
Also, this is now under a wrong license.

What are the options in case the original author has abandonded this? Can anybody re-create the MR under a new license with the commit message fixed, or is that an insurmountable license issue?

I guess we need specific permission from the author. maybe writing a mail to him/her would work? Then he/she can give permission (or decline it, which is unexpected i guess).

fiesh added a comment.Nov 25 2019, 3:01 AM

I guess we need specific permission from the author. maybe writing a mail to him/her would work? Then he/she can give permission (or decline it, which is unexpected i guess).

I already wrote an email a couple weeks ago but haven't received a reply so far. Let's hope they come back to this thread eventually.

estan added a subscriber: estan.Mar 15 2020, 2:16 AM

I have problems with clang-tidy-9.0.0 on OS X and assert macro usage.
see https://github.com/isocpp/CppCoreGuidelines/issues/1589

In which clang version is the patch included?

Hey, I'm back. I actually kind of forgot about this.

Unfortunately I cannot to continue the patch soon. How do I grant you full rights on this patch? If just saying here is enough, there you go, anyone can take it and change it.
If there is more bureaucracy to it, just let me know.

I'll be happy to see any of the code making into the tool.

Oh, and if I remember correctly, the reason for this patch was to avoid exactly the github issue mentioned here. I'm embarrassed I didn't explain that anywhere :(

@JonasToth could you please clarify if the original author's comment is sufficient grants of rights?

If so, from what I can tell if the commit message was adapted to reflect that this removes false positives with system macros like assert, this would be good to go?

fiesh added a comment.Aug 17 2020, 1:39 AM

Ping everybody? This is a rather important issue I think since it makes clang-tidy not usable in a lot of cases, and the fix would be all done. Please let's get this merged!

Ping everybody? This is a rather important issue I think since it makes clang-tidy not usable in a lot of cases, and the fix would be all done. Please let's get this merged!

Sorry for missing this before -- it's totally fine to pick up someone else's patch if the original author isn't able to work on it. IANAL, so I can't comment on the licensing question with any authority, but given that the original author says they're fine giving up full rights to their patch, that seems sufficient to me.

Given how long this review has sat idle for, I'd recommend starting a new review with a fully rebased patch, though.

Nice! I'm glad to see the patch being revived.
Thanks for that :)

Em seg, 5 de out de 2020 10:48, fiesh via Phabricator <
reviews@reviews.llvm.org> escreveu:

fiesh added a comment.

Continued in https://reviews.llvm.org/D88833

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D31130/new/

https://reviews.llvm.org/D31130

MTC added a subscriber: MTC.Aug 16 2021, 7:15 PM
brenoguim abandoned this revision.Mar 14 2023, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 9:56 AM