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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: erichkeane, ldionne.
aaron.ballman added a comment.

CC @erichkeane for attribute questions, and CC @ldionne for STL questions and design/usability of the proposed attribute

In D133029#3779663 <https://reviews.llvm.org/D133029#3779663>, @ilya-biryukov wrote:

> In D133029#3766058 <https://reviews.llvm.org/D133029#3766058>, @shafik wrote:
>
>> This old cfe-dev thread might be relevant: https://lists.llvm.org/pipermail/cfe-dev/2018-June/058338.html
>
> Thanks, the thread is definitely relevant.
>
>> It appears that folks take advantage of this working a lot. It sounds like your saying w/ constexpr std::vector this would be ill-formed in a constant expression context now. Which won't effect most of the existing code I think?
>
> Most of the code is not affected, but some code is and large codebases will definitely see quite some breakages.
> Note that this warning diagnoses is very pedantic in terms of implementing the rules from the standard. It fires in many places than don't actually break in libc++ and C++20 mode. On the flip side, it is really easy to implement.

I assume the standard rules you're talking about are: http://eel.is/c++draft/containers#vector.overview-4 (and other instances of similar wording for other containers)? If so, shouldn't this requirement be addressed by the STL implementation and not the compiler?

> In D133029#3778230 <https://reviews.llvm.org/D133029#3778230>, @dblaikie wrote:
>
>> any chance of generalizing this beyond a special case for `std::vector` - an attribute we could add to any class template (perhaps particular template parameters to the class template) to document the requirement? (is std::vector the only type in the standard library that has this property, or only the one people trip over most often)
>
> It should be easy to generalize, e.g. we could introduce an attribute to mark classes that want this behavior, something along the lines of:
>
>   // Option 1. Less general, easy, small overhead.
>   template <class T> struct [[member_access_requires_complete(T)]] my_array {};
>   // Option 2. More general, might be high-overhead depending on the actual expression.
>   template <class T> struct [[member_access_requires(sizeof (T))]] my_array {}; // checks for validity of expression on every member access?
>
> I am not sure this carries it weight for that particular use-case and there are other things to consider, e.g. how to report error messages.
> What is definitely useful and cheap is adding these warnings for more STL containers that have this requirement.
>
> We discussed this with @aaron.ballman in the last Clang C++ Language WG meeting and one idea that popped up is implementing a warning in Clang, but never actually surfacing it in the compiler, only in `clang-tidy`. I would be very comfortable making a `clang-tidy` warning like this as the bar is much lower than the compiler warnings.
> My proposal would be to allow making warnings that are disabled by default and even with `-Weverything`, the only way to enable them would be to pass `-W<warning-group>` explicitly or run a corresponding `clang-tidy` check. Alternatively, there will be no way to enable the warning in the compiler at all and the clang-tidy would be the only way to surface it.
>
> Obviously, these use-cases should be rare as most things can be implemented directly in `clang-tidy`, but in that particular case we need access to the compiler state in the middle of processing the TU, something that `clang-tidy` APIs do not provide. We should also be careful to make sure any warnings like this do not hurt performance of the compiler.
>
> @aaron.ballman does that look reasonable? I am about to try prototyping this. Are there any open questions we might want to agree on beforehand?

An attribute is an interesting idea, but I'm not certain it's fully necessary; I think it's effectively a slightly nicer form of: https://godbolt.org/z/ha6Tb1s4M

The reason why I'm wondering whether libc++ should handle this is: the requirements are library requirements, not compiler requirements, so I think the library should decide whether to enforce this rule or not. An attribute is an interesting idea in so much as it may help the library authors to implement this more easily, but the downside to the attribute is that it won't help libc++ out too much because they still need to work with other compilers that don't support the attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133029