[libcxx-commits] [PATCH] D122397: [libc++] Use __builtin_expect and __builtin_assume in _LIBCPP_ASSERT

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D122397#3407820 <https://reviews.llvm.org/D122397#3407820>, @ldionne wrote:

> In D122397#3406376 <https://reviews.llvm.org/D122397#3406376>, @Mordante wrote:
>
>> In D122397#3406340 <https://reviews.llvm.org/D122397#3406340>, @philnik wrote:
>>
>>> `__builtin_assume` seems to ignore most of our asserts, but it definitely doesn't hurt. (https://godbolt.org/z/W7Gb43cx5). LGTM.
>>
>> It might hurt when the assertion is wrong and the user builds without assertions.
>> https://godbolt.org/z/hsxhr9bWY
>> `test1` validates the pointer and `test2` doesn't since we told the compiler the if is always true.
>>
>> Note that I'm not against doing this. I only wonder whether or not we need an opt-out for vendors/users.
>
> IMO our assertions should simply not be wrong, otherwise we have a whole other problem when assertions are enabled. Assertions already encode things that, if they aren't satisfied, then the code exhibits UB. I'm not sure there is a lot of value in dialling how messed up the UB is going to be, so I'd suggest going for simplicity and adding a escape hatch if we have a reason to later on. And to that -- if someone came and asked for a escape hatch, I'd immediately ask them why they are not turning assertions on instead.

I agree assertions should always be right. But in my experience, bugs happen, and assertions can be wrong.
However I don't feel very strongly about this, I mainly want to have it considered. So I'm not objecting
against the simple approach without an escape hatch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122397/new/

https://reviews.llvm.org/D122397