Page MenuHomePhabricator

[libc++][NFC] Refactor the __enable_ifs in <string>
ClosedPublic

Authored by philnik on Feb 22 2023, 9:01 AM.

Details

Summary

This changes all __enable_ifs inside <string> to a common pattern. Specifically, it's always inside the template <> and uses the , int> = 0 style.

Diff Detail

Event Timeline

philnik created this revision.Feb 22 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 9:01 AM
philnik requested review of this revision.Feb 22 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2023, 9:01 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
EricWF requested changes to this revision.Tue, Mar 7, 1:54 PM
EricWF added a subscriber: EricWF.

This increases code duplication substantially.

Please say why this refactoring is an improvement.

This revision now requires changes to proceed.Tue, Mar 7, 1:54 PM
EricWF added a comment.Tue, Mar 7, 1:56 PM

@philnik Every single change needs a description. The description must include the motivation for making the change. What does in improve? What code does it break/fix? etc.

This is important for review, and its even more important down the road when we're trying to figure out why a particular change was made all that time ago.

philnik edited the summary of this revision. (Show Details)Wed, Mar 8, 9:23 AM
ldionne accepted this revision.Thu, Mar 16, 8:27 AM

I am fine with this since:

  1. It increases consistency with the way we use enable_if elsewhere, and
  2. the duplication is going away once we implement those member functions in the class, which we should do anyway.
libcxx/include/string
2053

There's a bit of duplication here, which is kind of annoying. However, refactoring this short function to be defined in-class will remove that duplication, and also remove some boilerplate which is not really useful.

This revision was not accepted when it landed; it landed in state Needs Review.Sun, Mar 19, 2:26 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.