[libcxx-commits] [PATCH] D132769: [2b/3][ASan][libcxx] std::basic_string annotations

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet marked an inline comment as done.
AdvenamTacet added inline comments.


================
Comment at: libcxx/include/string:1853
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_new(size_type __current_size) const _NOEXCEPT {
+      if (!__libcpp_is_constant_evaluated() && (__annotate_short_string_check() || __is_long()))
+        __annotate_contiguous_container(data(), data() + capacity() + 1, data() + capacity() + 1, data() + __current_size + 1);
----------------
philnik wrote:
> Isn't this redundant? `__annotate_short_string_check()` already checks this.
But for long strings it also has to be checked, as every string is long during constant evaluation.

```
    bool __is_long() const _NOEXCEPT {
        if (__libcpp_is_constant_evaluated())
            return true;
```
It has to be checked here and we cannot wait until the check in `__annotate_contiguous_container`, because during constant evaluation, next line would rise `cannot perform pointer arithmetic on null pointer`.

I don't want to remove it from `__annotate_short_string_check()`, because then returned values won't be correct.
And I don't want to remove it from `__annotate_contiguous_container` to make sure it can be simply called.
It also has no impact on performance.

Possibly it may be changed to `__annotate_short_string_check() || (!__libcpp_is_constant_evaluated() && __is_long())`, but I have a strong preference for current version, as that would suggest possibility of poisoning short string during constant evaluation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769