This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Replace enable_if with __enable_if_t in a few places
ClosedPublic

Authored by philnik on Jun 22 2022, 4:29 PM.

Diff Detail

Event Timeline

philnik created this revision.Jun 22 2022, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 4:29 PM
philnik requested review of this revision.Jun 22 2022, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 4:29 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

In general I'm happy. But please avoid unrelated NFC code changes is these kind of commits. Especially since the title nor the description mentions them. (Thus some minor whitespace fixes are not an issue.)

libcxx/include/__split_buffer
121

I'm not happy with this change in this commit, please make a separate review for it.

jloser added a subscriber: jloser.Jun 24 2022, 12:27 PM

This isn't quite an NFC I think. Specifically, __enable_if_t is marked with _LIBCPP_NODEBUG whereas enable_if is not. The new behavior proposed in this patch results in suppressing debugging information for these functions. Does anyone rely on this behavior/care?

philnik marked an inline comment as done.Jun 27 2022, 6:47 AM

This isn't quite an NFC I think. Specifically, __enable_if_t is marked with _LIBCPP_NODEBUG whereas enable_if is not. The new behavior proposed in this patch results in suppressing debugging information for these functions. Does anyone rely on this behavior/care?

IIUC this only suppresses debug information of the __enable_if_t itself. It doesn't suppress any other debug information. So Instead of the function debug info being enable_if<some stuff>::type you get just void. So yes, it technically changes something, but I don't think anybody really cares for this part of the debug info.

philnik updated this revision to Diff 440203.Jun 27 2022, 6:47 AM
  • Address comments
ldionne accepted this revision.Jun 30 2022, 5:38 AM

LGTM with comments applied.

libcxx/include/__tree
1285

This (and line 1295) makes the indentation inconsistent in the file. Please keep the 4 space indentation for consistency.

libcxx/include/array
441

Here and in other return types where you did the same transformation, please keep the explicit void type at the end. I know it's redundant, but it helps to see what is the intended return type of the function. We should also refactor these enable_ifs to use template parameters instead.

This revision is now accepted and ready to land.Jun 30 2022, 5:38 AM
ldionne added inline comments.Jun 30 2022, 5:46 AM
libcxx/include/__split_buffer
233

Also, I think this should be part of D128646.

philnik updated this revision to Diff 441986.Jul 3 2022, 4:33 PM
philnik marked 3 inline comments as done.
  • Address comments
This revision was landed with ongoing or failed builds.Jul 4 2022, 2:09 AM
This revision was automatically updated to reflect the committed changes.