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:
> > > > > 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.
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
