Page MenuHomePhabricator

Implements CWG 1601 in [over.ics.rank/4.2]
ClosedPublic

Authored by Mordante on Aug 3 2019, 2:58 AM.

Details

Summary

The overload resolution for enums with a fixed underlying type has changed in the C++14 standard. This patch implements the new rule.

Note: I don't have access to the CWG 1601 paper, but based on https://en.cppreference.com/w/cpp/language/overload_resolution I applied the fix retroactively to C++11.

Diff Detail

Repository
rL LLVM

Event Timeline

Mordante created this revision.Aug 3 2019, 2:58 AM
Mordante marked an inline comment as done.Aug 4 2019, 10:07 AM
Mordante added inline comments.
clang/www/cxx_dr_status.html
3 ↗(On Diff #213185)

I'll properly update this file as explained in D65696.

Mordante updated this revision to Diff 215457.Aug 15 2019, 12:48 PM

Add the proper markers in the unit test to update cxx_dr_status.html. As discussed on IRC; the up to date cwg_index.html is not public, so I only updated the unit test and removed the changes to cxx_dr_status.html.

rsmith added inline comments.Aug 15 2019, 1:33 PM
clang/lib/Sema/SemaOverload.cpp
3758 ↗(On Diff #215457)

Underlaying -> Underlying (here and below)

3821–3828 ↗(On Diff #215457)

I think this would be cleaner if you moved the comparison of the target type against the underlying type into getFixedEnumUnderlyingType, and changed it to return an enumeration of "not a fixed enum promotion", or "fixed enum promotion to underlying type", or "fixed enum promotion to promoted underlying type".

clang/test/CXX/drs/dr16xx.cpp
26 ↗(On Diff #215457)

No need for the "c++11" marker here. (We accept fixed underlying types in C++98 as an extension, and your change applies there too.)

Mordante marked 4 inline comments as done.Aug 17 2019, 3:20 AM
Mordante added inline comments.
clang/test/CXX/drs/dr16xx.cpp
26 ↗(On Diff #215457)

I will also update dr685.

Mordante updated this revision to Diff 215736.Aug 17 2019, 3:23 AM
Mordante marked an inline comment as done.
Mordante edited the summary of this revision. (Show Details)

Implemented the changes requested by @rsmith.

rsmith added inline comments.Aug 29 2019, 1:24 PM
clang/lib/Sema/SemaOverload.cpp
3776 ↗(On Diff #215736)

== on QualType compares the precise expression of the type, rather than checking whether types are semantically the same. Use S.Context.hasSameType instead to make this more robust (though I think the difference is not observable because the ToType in the case of a fixed enum promotion always happens to come exactly from calling getIntegerType()).

3831–3838 ↗(On Diff #215736)

This logic can be simplified:

if (FEP1 != FixedEnumPromotion::None && FEP2 != FixedEnumPromotion::None && FEP1 != FEP2)
  return FEP1 == FixedEnumPromotion::ToUnderlyingType ? ImplicitConversionSequence::Better
                                                      : ImplicitConversionSequence::Worse;
Mordante updated this revision to Diff 218216.Aug 31 2019, 6:06 AM

Addresses the review remarks:

  • Use S.Context.hasSameType for the comparision
  • Simplify an if statement
Mordante marked 2 inline comments as done.Aug 31 2019, 6:06 AM
rsmith accepted this revision.Oct 4 2019, 11:07 AM

Thanks!

This revision is now accepted and ready to land.Oct 4 2019, 11:07 AM

Thanks for the review. Can you commit the patch since I don't have commit access?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2019, 11:49 AM