[PATCH] [StaticAnalyzer] New checker Sizeof on expression

[Anders Rönnholm]

-----Original Message-----
From: Anders Rönnholm 
Sent: den 10 december 2013 13:38
To: cfe-commits at cs.uiuc.edu
Cc: Daniel Marjamäki; Anna Zaks; Jordan Rose (jordan_rose at apple.com); 'David Blaikie'; 'Richard Smith'; 'Matt Calabrese'
Subject: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression

Are you OK to commit this patch or do you see more issues?

//Anders
________________________________________
Från: Anna Zaks [ganna at apple.com]
Skickat: den 9 december 2013 23:06
Till: Daniel Marjamäki; Anders Rönnholm
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression

CC-ing Anders.
On Dec 4, 2013, at 11:35 AM, Anna Zaks <ganna at apple.com<mailto:ganna at apple.com>> wrote:

Off the list.

Daniel,

I don't see any outstanding issues with the patch. I would reply to this email, CC all the people who reviewed this patch and ask if it is OK to commit it.

Cheers,
Anna.
On Nov 27, 2013, at 7:27 PM, Matt Calabrese <rivorus at gmail.com<mailto:rivorus at gmail.com>> wrote:

On Wed, Nov 27, 2013 at 8:54 AM, Daniel Marjamäki <Daniel.Marjamaki at evidente.se<mailto:Daniel.Marjamaki at evidente.se>> wrote:
Hello!

>> Sure. I think it's even fine for a compiler warning, since sizeof(size_t) is a clearer way to get the same result.

> I know I'm a little late here, but I think the reason why you wouldn't want to use sizeof(size_t) is that you need to include certain standard library headers to pull in size_t, I.E. <cstddef>. The same goes for ptrdiff_t. Doing sizeof(sizeof(whatever)) frees you from that requirement.

To me it sounds like they prefer misleading code instead of an #include then. I wonder if you or anybody else here have seen such project. Or was it an idea when there could in theory be false positives?

I'm just looking for potential false positives and trying to understand the rationale of such valid code. I'm not one of the people who has done sizeof(sizeof(whatever)), so I can't speak for them, but I don't think it's that unreasonable to want to avoid including a standard header file. One reason might be that you simply might not have a standard library implementation available. Not wanting to pull in unnecessary code into translation units that depend on it is another.

As for the SFINAE concern, that is a use-case that I genuinely have encountered (and many have encountered, specifically when implementing type traits for automatically detecting expression validity, such as if + is defined for a given pair of operands). That said, regarding SFINAE, there are alternatives that don't require sizeof and that are generally better overall anyway. In particular, using decltype instead of sizeof in such situations is the better approach since decltype works in places where the expression results in void (whereas the sizeof version would break) and it doesn't require instantiating the argument whereas sizeof does. Using decltype would also not trigger this warning. Simply not doing the check in such SFINAE contexts obviously gets rid of such false positives too, but it's worth noting that these uses of sizeof are still dubious even with respect to SFINAE in modern code. Because of that, recommending decltype instead of sizeof could even be a possibility here rather than not producing a warning during SFINAE at all. This has the benefit of alerting people to the more subtle bugs coming from using sizeof as opposed to decltype for expression validity tests.

I want to get this warning into clang. I wonder if it will make you happy if we only write the warning only when we see a #include anywhere in the file. If we see an #include then the project uses includes. Most real files include files so this heuristic wouldn't hurt the hit rate much imho.

I'm happy already without stuff like that. Again, I'm just trying to enumerate and understand the potential false positives, especially if it makes it possible to recommend an alternative when the warning is triggered.

--
-Matt Calabrese
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


-------------- next part --------------
A non-text attachment was scrubbed...
Name: sizeofonexpression.diff
Type: application/octet-stream
Size: 7634 bytes
Desc: sizeofonexpression.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131220/10a959db/attachment.obj>