Mordante added a comment.
In D86559#2239013 <https://reviews.llvm.org/D86559#2239013>, @aaron.ballman
wrote:
> I'd like to understand your reasoning for ignore + diagnose as opposed to
> count attrs (+ optionally diagnose) or other strategies. I can see pros and
> cons to basically anything we do here.
>
> If we decide to ignore conflicting likelihoods, then I agree we should issue
> a diagnostic. However, I suspect that's going to come up frequently in
> practice because people are going to write macros that include these
> attributes. For instance, it's very sensible for a developer to put
> [[unlikely]] in front of an `assert` under the assumption this is telling the
> compiler "this assertion isn't likely to happen" when in fact it's saying
> "this branch containing the assertion is unlikely to be taken".
This macro is example why I dislike the counting. If `MyAssert` has an
`[[unlikely]]` attribute, then changing the number of `MyAssert` statements
will influence the likelihood of the branches taken.
> This is one of the reasons why the GCC behavior of allowing these attributes
> on arbitrary statements feels like an awful trap for users to get into. If
> the attribute only applied to compound statements following a flow control
> construct, I think the average programmer will have a far better chance of
> not using this wrong. (I know that I said in the other thread we should match
> the GCC behavior, but I'm still uncomfortable with it because that behavior
> seems to be fraught with dangers for users, and this patch introduces new
> sharp edges, as @Quuxplusone pointed out.)
I'm also not fond of GCC's implementation, but I think their implementation
conforms with the standard. In hindsight I think it indeed makes more sense to
only allow it on compound statements. That will require thinking about how to
mark multiple cases as `[[likely]]` when falling through:
switch(a) {
case '0':
case '1':
case '2': [[likely]] { return 'a' - '0'; } // All 3 cases likely?
case '3': // neutral?
case '4': [[likely]]() // likely?
case '5': [[unlikely]] {return 'a' - '0'; } // unlikely?
}
Of course this can be solved by only allowing it on the case labels:
switch(a) {
[[likely]] case '0':
[[likely]] case '1':
[[likely]] case '2': { return 'a' - '0'; } // All 3 cases likely
case '3': // neutral
[[likely]] case '4': // likely
[[unlikely case '5': {return 'a' - '0'; } // unlikely
}
I fully agree the behaviour mandated by the standard is way too complex and
user unfriendly. It would have been nice if there were simpler rules, making it
easier to use and to teach. Still I think it would be best to use the complex
approach now, since that's what the standard specifies. During that process we
can see whether there are more pitfalls. Then we can discuss it with other
vendors and see whether we can change the wording of the standard. Do you agree?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86559/new/
https://reviews.llvm.org/D86559
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
