ahatanak 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())) {
----------------
aaron.ballman wrote:
> 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. :-)
No problem and thank you for the detailed explanation of the rules in the
standard. I've updated the summary based on the discussion we had.
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
