Page MenuHomePhabricator

[libc++][NFC] Refactor enable_ifs in vector
ClosedPublic

Authored by philnik on Aug 14 2022, 4:22 PM.

Details

Summary

Using the enable_if_t<..., int> = 0 style has the benefit that it works in all cases and makes function declarations easier to read because the function arguments and return type and SFINAE are separated. Unifying the style also makes it easier for people not super familiar with SFINAE to make sense of the code.

Diff Detail

Event Timeline

philnik created this revision.Aug 14 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 4:22 PM
philnik requested review of this revision.Aug 14 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2022, 4:22 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 452632.Aug 15 2022, 5:10 AM
  • Fix C++03
Mordante added inline comments.Aug 15 2022, 9:41 AM
libcxx/include/vector
426

I find it a bit odd to add _LIBCPP_HIDE_FROM_ABI in an NFC patch, it's also not mentioned in the commit message.

676

Why the class = in this case?

1000

Here you removed and enable_if, is that really intended?
I'm a bit concerned the CI passes with this change.

huixie90 accepted this revision as: huixie90.Aug 15 2022, 12:23 PM
huixie90 added a subscriber: huixie90.

LGTM with comments addressed. lots of enable_ifs are checking the cpp 17 forward iterator. I wonder if we can relax it to cpp 20 forward iterator. (cpp 17 forward iterator requires reference being a reference type, which is not important for these vector members IIUC)

libcxx/include/vector
1000

Technically the enable_if still applies since the default template argument with enable_if already exists in the function declaration few hundred lines above. But I agree it is not obvious since the declaration and definition are few hundred lines apart from each other.

Also I agree that using non-type template is better because it is consistent with others, although technically it is not required to use non-type template parameter since there is only one overload

var-const added inline comments.Aug 16 2022, 3:39 PM
libcxx/include/vector
426

+1 -- this looks like a good change, but it should probably be in a separate patch.

1000

Consider also "inlining" the definitions of the affected functions so we can avoid having to deal with SFINAE twice.

philnik added inline comments.Aug 18 2022, 5:35 AM
libcxx/include/vector
426

See D132016.

1000

I'm planning to do that later.

philnik updated this revision to Diff 454320.Aug 21 2022, 9:37 AM
philnik marked 5 inline comments as done.
  • Rebased
  • Use enable_if_t<..., int> = 0 style everywhere
Mordante accepted this revision.Aug 29 2022, 9:38 AM

LGTM!

libcxx/include/vector
1000

Technically the enable_if still applies since the default template argument with enable_if already exists in the function declaration few hundred lines above. But I agree it is not obvious since the declaration and definition are few hundred lines apart from each other.

Also I agree that using non-type template is better because it is consistent with others, although technically it is not required to use non-type template parameter since there is only one overload

Thanks I missed that in the original review.

This revision is now accepted and ready to land.Aug 29 2022, 9:38 AM
var-const requested changes to this revision.Aug 29 2022, 8:16 PM

Can you please add the rationale for this change to the patch description? Other than that and the couple optional comments, LGTM.

libcxx/include/vector
395–396

It looks like there are 2 same conditions repeated throughout the file:

__is_exactly_cpp17_input_iterator<_InputIterator>::value &&
is_constructible<value_type, typename iterator_traits<_InputIterator>::reference>

and

__is_cpp17_forward_iterator<_ForwardIterator>::value &&
is_constructible<value_type, typename iterator_traits<_ForwardIterator>::reference>

Perhaps create helper traits for those, e.g. IsConstructibleFromInputIterator/IsConstuctibleFromForwardIterator? It should help cut down on the boilerplate, particularly when repeating the enable_if in the function definition. If you decide to do this, it's likely best to do in a follow-up.

1088

Nit: it's unfortunate that now some of the conditions use _Tp and others value_type, just from the consistency perspective. I presume it's not possible to use value_type here because it's not in scope yet at this point. Would it make sense to go the other way round and use _Tp in other places instead of value_type?

This revision now requires changes to proceed.Aug 29 2022, 8:16 PM
philnik edited the summary of this revision. (Show Details)Sep 6 2022, 10:23 AM
philnik marked 2 inline comments as done.
philnik added inline comments.
libcxx/include/vector
395–396

I agree that it should be done in a follow-up. Though I'm not sure it's worth it once the function definitions are inlined.

1088

This problem should also not exist anymore once the definitions are inlined.

var-const accepted this revision.Sep 19 2022, 3:45 PM
This revision is now accepted and ready to land.Sep 19 2022, 3:45 PM
This revision was landed with ongoing or failed builds.Sep 20 2022, 1:06 AM
This revision was automatically updated to reflect the committed changes.

I am 90% certain that this caused the following change of behavior: https://godbolt.org/z/WKEadqf8e. Long story short, constructing a vector with iterators with a mismatched constness used to work, and now it doesn't anymore. GCC doesn't support it either.

@philnik Since this is almost certainly unintended, can you please figure out what the valid standard behavior is here and add a test if the standard mandates that?

I am 90% certain that this caused the following change of behavior: https://godbolt.org/z/WKEadqf8e. Long story short, constructing a vector with iterators with a mismatched constness used to work, and now it doesn't anymore. GCC doesn't support it either.

@philnik Since this is almost certainly unintended, can you please figure out what the valid standard behavior is here and add a test if the standard mandates that?

Also, I noticed because it broke some code. It did not result in any significant breakage, but if the new behavior is the correct one and we keep it, we should add a release note just as a heads up.