This is an archive of the discontinued LLVM Phabricator instance.

Direction for a fix of PR 41130
AbandonedPublic

Authored by mclow.lists on Mar 21 2019, 11:32 AM.

Details

Summary

https://llvm.org/PR41130

This is NOT a final patch.
I'm looking for suggestions on how to improve it.
But .. it does solve the OP's problem, and introduces no new ones that I can find.

Changes that have to be made before this is landed:

  • Naming; we obviously don't want __Fooby, __Fooby2, etc in our code base.
  • Removing the old __duration_divide_result and __duration_divide_result_imp classes which are no longer used
  • Looking at operator * which probably needs the same treatment.
  • Do we want to grab __has_common_type and put it in <type_traits> where other parts of libc++ can use it?

The reason that this is so pernicious is that common_type will blow up on you in C++03, rather than just SFINAE-ing away. So, you'd better be sure that there is a common type before you ask.

Note that the approach in this patch fails for C++03 (for the cases in the bug report). I don't know how to fix that. But, this patch does NOT break any existing cases in C++03 (Which Howard's 'lightly tested" patch did).

Diff Detail

Event Timeline

mclow.lists created this revision.Mar 21 2019, 11:32 AM

We should just fix common type in C++03.

We should just fix common type in C++03.

Did you commit your fix to common_type? I think this would indeed solve this whole issue, right? If so, we should only commit the tests added by this patch.

Does this make sense @mclow.lists ?

We should just fix common type in C++03.

Did you commit your fix to common_type? I think this would indeed solve this whole issue, right? If so, we should only commit the tests added by this patch.

Does this make sense @mclow.lists ?

No, I don't think it does.
I've been waiting for @EricWF 's common_type fixes to land.
Once that happens, the problem in the original bug report will still remain.

This patch will need to be reworked - and will become *much* simpler.

mclow.lists abandoned this revision.Apr 1 2019, 9:42 AM

I committed the tests and an alternate fix in r357410