aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a subscriber: erichkeane.
aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaInit.cpp:4506
!S.Context.hasSameUnqualifiedType(E->getType(), DestType) &&
- (E->getType()->isIntegralOrEnumerationType() ||
+ (E->getType()->isIntegralOrUnscopedEnumerationType() ||
E->getType()->isFloatingType())) {
----------------
ahatanak wrote:
> ahatanak wrote:
> > aaron.ballman wrote:
> > > ahatanak wrote:
> > > > ahatanak wrote:
> > > > > aaron.ballman wrote:
> > > > > > ahatanak wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This doesn't match the comments immediately above here and I
> > > > > > > > don't think is the correct fix.
> > > > > > > >
> > > > > > > > We're handling this case:
> > > > > > > > http://eel.is/c++draft/dcl.init.list#3.8
> > > > > > > >
> > > > > > > > A scoped enumeration has a fixed underlying type
> > > > > > > > (https://eel.is/c++draft/dcl.enum#5.sentence-5). The
> > > > > > > > initializer list has a single element and that element can be
> > > > > > > > implicitly converted to the underlying type (`int` in all of
> > > > > > > > the test cases changed in this patch). And this is a direct
> > > > > > > > initialization case, so I think we should be performing the
> > > > > > > > conversion here rather than skipping to the next bullet.
> > > > > > > Can scoped enums be implicitly converted to integer types?
> > > > > > > Unscoped enums can be converted to an integer type, but I don't
> > > > > > > see any mention of scoped enums here:
> > > > > > > https://eel.is/c++draft/conv.integral
> > > > > > >
> > > > > > > It seems that the original paper was trying to change the rules
> > > > > > > about conversions from the underlying type to a scoped enum. It
> > > > > > > doesn't look like it's allowing conversion from a scope enum to
> > > > > > > another scope enum.
> > > > > > >
> > > > > > > https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0138r2.pdf
> > > > > > > Can scoped enums be implicitly converted to integer types?
> > > > > > > Unscoped enums can be converted to an integer type, but I don't
> > > > > > > see any mention of scoped enums here:
> > > > > > > https://eel.is/c++draft/conv.integral
> > > > > >
> > > > > > Correct, they cannot be implicitly converted to an integer.
> > > > > >
> > > > > > > It seems that the original paper was trying to change the rules
> > > > > > > about conversions from the underlying type to a scoped enum. It
> > > > > > > doesn't look like it's allowing conversion from a scope enum to
> > > > > > > another scope enum.
> > > > > >
> > > > > > Agreed, however, I think where we want this to fail is below in the
> > > > > > attempt at conversion. "v can be implicitly converted to U" is the
> > > > > > part that should be failing here, and we're now skipping over the
> > > > > > bit of code that's checking whether the implicit conversion is
> > > > > > valid.
> > > > > Is the code below checking whether the implicit conversion is valid?
> > > > > It looks like it's assuming the implicit conversion is valid and
> > > > > adding an implicit conversion sequence based on that assumption. If
> > > > > the source is an integer, unscoped enum, or floating type, the
> > > > > implicit conversion that is performed later should succeed except
> > > > > when there is narrowing.
> > > > >
> > > > > Or are you suggesting we should add a check to
> > > > > `Sema::PerformImplicitConversion` that rejects conversions from
> > > > > scoped enums to other types? It seems to me that it's better to
> > > > > detect the error earlier.
> > > > Alternatively, we can emit a diagnostic in the code below that
> > > > specifically calls out conversion from scoped enums to integer types.
> > > > Is the code below checking whether the implicit conversion is valid?
> > >
> > > It's forming the conversion sequence as-if it must be valid, but that
> > > causes us to get the right diagnostics. We do the same for narrowing
> > > float conversions:
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L4521
> > > and I would expect us to then need changes so we get to here:
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaInit.cpp#L8478
> > But a conversion from a scoped enum to another scoped enum or its
> > underlying type isn't a narrowing conversion unless the conversion from the
> > underlying type is narrowing. I guess the current code is forming the
> > conversion sequence as if it is valid when the source type is a floating
> > type just to call `DiagnoseNarrowingInInitList`. @rsmith, any comments?
> >
> > If we want to detect the invalid conversion while performing conversion,
> > shouldn't the call to `PerformImplicitConversion`, which is called before
> > reaching the call to `DiagnoseNarrowingInInitList`, fail? Why should it
> > succeed?
> >
> > https://github.com/llvm/llvm-project/blob/7689c7fc9e08cc430daca3714bcffdd00fd538bd/clang/lib/Sema/SemaInit.cpp#L8467
> >
> > But I think the invalid conversion should be detected at the very beginning
> > of the function before conversion is attempted where it checks whether the
> > initialization sequence is invalid
> > (https://github.com/llvm/llvm-project/blob/7689c7fc9e08cc430daca3714bcffdd00fd538bd/clang/lib/Sema/SemaInit.cpp#L8020).
> > That can be done by calling `Sequence.SetFailed` when the source type is a
> > scoped enum.
> >
> Also, it's not clear to me why the diagnostic this patch emits (`cannot
> initialize a variable of type 'test12::B' with an lvalue of type
> 'test12::A'`) isn't right. It's kind of generic, but it doesn't seem
> incorrect to me. What is the correct diagnostic in this case?
Given your example (but with names less likely to cause confusion):
```
enum class FirstEnum;
enum class SecondEnum;
FirstEnum FirstValue;
SecondEnum SecondValue{FirstValue};
```
Starting from recognizing that we're performing list initialization, we get to:
http://eel.is/c++draft/dcl.init.list#3.8
Based on our example, T is `SecondEnum`, U is `int`, v is `FirstValue`. The
question then becomes can you implicitly convert `FirstValue` to `int` and the
answer is no. The diagnostic we form in that case is "cannot initialize a
variable of type '<type1>' with an lvalue of type '<type2>'". e.g.,
https://godbolt.org/z/an38EK3cs
So I think I was wrong; based on the comments on line 4508, it looks like we do
*not* want to get into that `if` block but instead let the general
single-element case below handle it. (I had missed that last sentence before
and that turned out to be an important one.) Based on the diagnostic given when
we do that (as your patch currently does), the diagnostic is what I'd expect us
to generate.
I'm very sorry for the back and forth on this, but I *think* your patch is
actually correct as-is. CC @erichkeane and @rsmith for a second opinion given
that I already messed the logic up once before. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126084/new/
https://reviews.llvm.org/D126084
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
