[clang] [Clang] Fix __is_array returning true for zero-sized arrays (PR #86652)
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 26 09:43:01 PDT 2024
AaronBallman wrote:
> > > According to the spec it's ill-formed, so I'm not sure it falls under any sensible category.
> >
> >
> > It's an extension we support so it's up to us to decide what sensible is.
>
> Sure. If we end up putting it in the array category we should probably coordinate with GCC.
>
> > > For T[0] this returns false.
> >
> >
> > Understood, but why is this not the bug to be fixed? (It would also fix the `takes_an_array` case as well.)
>
> Because I don't think we can. AFAICT this is valid C++:
>
> ```c++
> template <unsigned Size, class = int[Size]>
> int func() {
> return 1;
> }
>
> template <unsigned Size>
> int func() {
> return 0;
> }
> ```
>
> If clang accepted `int[0]` in this context the observable behaviour would change.
Hmm, I think you're right: https://godbolt.org/z/PjGoc3Yob, the `static_assert(func<0>() == 0);` case would start to fail to compile, similar to the code using `func<1>()`.
This doesn't leave us with good options, does it? :-(
I think we need to consider the following things:
* Should `Ty[0]` and `Ty[]` be handled the same way? (Remember, we support `[0]` in structures as a sort of flexible array member and we support flexible array members in C++ as an extension.)
* We should probably change the value of `__is_bounded_array` at the same time as `__is_array`.
* What should the behavior of `__is_aggregate` be?
* What about other type traits (`__is_compound`, `__is_abstract`, etc)? Basically, what's the principle we're using for this type?
> > > We don't use the trait currently in libc++ because of this bug and GCC only added it in trunk (and have the same bug), so probably not.
> >
> >
> > For example, it seems to be used in Unreal Engine: https://github.com/windystrife/UnrealEngine_NVIDIAGameWorks/blob/b50e6338a7c5b26374d66306ebc7807541ff815e/Engine/Source/Runtime/Core/Public/Templates/UnrealTemplate.h#L128
>
> If you think it's a significant enough change that we should add an ABI tag I can do that.
I don't have a good feeling one way or the other yet, so let's hold off for the moment and see what the final scope of the changes are before making a decision.
https://github.com/llvm/llvm-project/pull/86652
More information about the cfe-commits
mailing list