C++: Suppress ExprHasNoEffect on template code#2399
Conversation
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Took a quick look, looks sensible but I haven't thought very hard about it yet.
| not peivc.getEnclosingFunction().isDefaulted() and | ||
| not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and | ||
| not peivc.isFromTemplateInstantiation(_) and | ||
| not peivc.isFromUninstantiatedTemplate(_) and |
There was a problem hiding this comment.
I'm a bit surprised we didn't already have this exception, given the number of other exceptions!
| @@ -84,6 +84,7 @@ where | |||
| not peivc.getEnclosingFunction().isDefaulted() and | |||
| not exists(Macro m | peivc = m.getAnInvocation().getAnExpandedElement()) and | |||
| not peivc.isFromTemplateInstantiation(_) and | |||
There was a problem hiding this comment.
Is this line still contributing positively?
There was a problem hiding this comment.
Yes - it handles cases where a call in a void context has a pure target in some instantiations and a target with side effects in other instantiations. Ideally we'd handle that with a forall, but I don't think we have any way of matching the corresponding elements in different instantiations.
There was a problem hiding this comment.
The unusual thing here is that we're excluding both uninstantiated templates and instantiated templates. That means we're excluding all template code. We usually exclude either one or the other, but in this case there has been false positives in both types of code.
We could try something elaborate like counting the number of potential alerts in the template and its instantiations and reporting an alert only if those counts match, but I don't think this query is important enough for us to spend time on that.
There was a problem hiding this comment.
OK, sounds like this is a reasonable solution then.
3 participants
Fixes #2298 (see https://github.slack.com/archives/CP0MHKV6E/p1574086207203400 for a diagnosis).
Supersedes #2384.