[libcxx-commits] [PATCH] D136765: [ASan][libcxx] Annotating std::vector with all allocators

[Manoj Gupta via Phabricator via libcxx-commits]

manojgupta added a comment.

In D136765#4205989 <https://reviews.llvm.org/D136765#4205989>, @philnik wrote:

> In D136765#4205962 <https://reviews.llvm.org/D136765#4205962>, @hans wrote:
>
>> In D136765#4203247 <https://reviews.llvm.org/D136765#4203247>, @ldionne wrote:
>>
>>> I do agree however that if someone (in this case Chrome) is using a not-yet-released snapshot of `trunk` that happens to have `_LIBCPP_CLANG_VER >= 1600` yet isn't something we've released as LLVM 16, then it's probably not reasonable for us to try to support that. Imagine coming up with that policy, I think it would mean that we can't ever rely on `_LIBCPP_CLANG_VER >= 1600` during the libc++ 16 release, which would be pretty unfortunate.
>>
>> (FWIW it's ChromeOS, not Chrome, that's using an older Clang version. And if necessary they can probably patch in the needed compiler-rt changes. Also, https://libcxx.llvm.org/ technically says that clang "16-git" is supported, which is what they are using ;)
>
> It's "16-git" as-in current trunk, not some random version from a few months ago.
>
>> I'll stop harping on about this, but I think gating functionality on _LIBCPP_CLANG_VER isn't great for the reason we've seen here. (And Eric predicted it in his "It smells weird, and it looks weird, and I think it'll lead to bugs." comment). I'm pleased to see that it's currently only used in two places in libc++, one of which is for a deprecation warning. As mentioned above, the best would be if compiler-rt exposed some kind of feature check for this, but if we can't have that, I think `>= 17` is the way to go.
>
> I don't think `>= 17` is an option. You are basically asking us to keep the code around for another release because someone used some wacky unsupported configuration. Either live at head or use releases. They exist for a reason.

809855b56f06dd7182685f88fbbc64111df9339a <https://reviews.llvm.org/rG809855b56f06dd7182685f88fbbc64111df9339a> changed clang version in trunk to 16.
https://reviews.llvm.org/rGdd1b7b797a11 added the API that this patch is using.

This change means that any clang released in the range  809855b56f06dd7182685f88fbbc64111df9339a <https://reviews.llvm.org/rG809855b56f06dd7182685f88fbbc64111df9339a>..dd1b7b797a11 <https://reviews.llvm.org/rGdd1b7b797a116eed588fd752fbe61d34deeb24e4>  is now BAD for building libc++.

$ git log 809855b56f06dd7182685f88fbbc64111df9339a <https://reviews.llvm.org/rG809855b56f06dd7182685f88fbbc64111df9339a>..dd1b7b797a11 <https://reviews.llvm.org/rGdd1b7b797a116eed588fd752fbe61d34deeb24e4> --oneline | wc -l
9006

IMO, this is an large number of commits and punishes people that made clang releases in this range.

I'd however like to steer out of the debate by cherry-picking https://reviews.llvm.org/rGdd1b7b797a11 to ChromeOS clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136765