[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector<T> with incomplete T

Fri Sep 9 07:24:49 PDT 2022

aaron.ballman added a comment.

In D133029#3779970 <https://reviews.llvm.org/D133029#3779970>, @ldionne wrote:

> Just my .02, but I am conflicted between:
>
> 1. Simply not doing anything -- the diagnostic users get when they violate the requirement currently is probably not that bad? I did see this breakage a bit in our internal code base as well, but it was easy to fix and there were not many instances.

I think the issue this is solving is that we are not diagnosing the library requirement properly because we would miss cases where the incomplete type wasn't actually used by the member function (aka, it wasn't caught by any constant expression evaluation requirements in the compiler).

> 2. Adding the attribute that was suggested and using it in libc++. On compilers that don't support the attribute, we'd simply be less pedantic.
>
> The one thing I'd rather not do is `static_assert(__is_complete<_Tp>::value)` in all the `std::vector` member functions, IMO that adds complexity and reduces readability for a really marginal gain.

Because this is a libc++ requirement, I will defer to what you think is best here. The only danger I see of not diagnosing this is that users may potentially run into a portability issue to another STL, which might make sense as a clang-tidy warning, but seems like it'd be pretty low-value as I suspect most STL implementations rely on the compiler to issue a diagnostic for the incomplete type when it matters and so all the implementations are likely to behave roughly the same in terms of behavior. So I'm leaning towards the "do nothing" option.

I'm not certain an attribute makes a whole lot of sense given that the requirement is pretty specific to the STL.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133029