Page MenuHomePhabricator

[clang] use correct builtin type for defaulted comparison analyzer
ClosedPublic

Authored by mizvekov on Sat, Jun 5, 6:33 PM.

Details

Summary

Fixes PR50591.

When analyzing classes with members which have user-defined conversion
operators to builtin types, the defaulted comparison analyzer was
picking the member type instead of the type for the builtin operator
which was selected as the best match.

This could either result in wrong comparison category being selected,
or a crash when runtime checks are enabled.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Sat, Jun 5, 6:33 PM
mizvekov requested review of this revision.Sat, Jun 5, 6:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Jun 5, 6:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Since we can still crash with user defined conversion operators for any builtin types not supported by getComparisonCategoryForBuiltinCmp (many others), I will make a follow up patch to just replace that assert with a deleted result.

@rsmith what do you think of the following strategy as a follow up to this patch:

  • First step we replace the assert at line 7866 with a deleted result. I don't really think it's sustainable to keep returning incorrect results / crashing for builtin types that are to be supported in the future. We create a new diagnostic for unimplemented builtin type for three-way comparison or something along these lines. This will gloss over for now function pointers, the only other type which we are adding to the candidate set which we should not be doing. As a consequence, we produce slightly unhelpful diagnostics here, for now.
  • Second step we modify AddTypesConvertedFrom so it can ignore function pointers, when used for purposes of building a candidate set for three-way comparison. Here, we would correct the diagnostics from step one.

Thoughts?

rsmith accepted this revision.Wed, Jun 16, 10:24 AM
This revision is now accepted and ready to land.Wed, Jun 16, 10:24 AM
This revision was landed with ongoing or failed builds.Wed, Jun 16, 5:08 PM
This revision was automatically updated to reflect the committed changes.