Consistently use "int = 0" SFINAE by miscco · Pull Request #226 · microsoft/STL

miscco · 2019-10-28T14:12:03Z

Description

I had too much time at hand, so this addresses #187

I grepped for all occurences of class = and fixed those that were followed by an enable_if

Checklist

If you're unsure about a box, leave it unchecked. A maintainer will help you.

I understand README.md. I also understand that acceptance of
community PRs will be delayed until the test and CI systems are online.
Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before CI is online, leave this unchecked for
initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository and
the C++ Working Draft as a reference (and any other cited standards).
If they were derived from a project that's already listed in NOTICE.txt,
that's fine, but please mention it. If they were derived from any other
project (including Boost and libc++, which are not yet listed in
NOTICE.txt), you must mention it here, so we can determine whether the
license is compatible and what else needs to be done.

BillyONeal · 2019-10-28T19:32:00Z

I've tried to do this before and been stopped by compiler bugs every time. Maybe this time will work :)

BillyONeal · 2019-10-28T19:51:23Z

/azp run

azure-pipelines · 2019-10-28T19:51:32Z

Azure Pipelines successfully started running 1 pipeline(s).

BillyONeal · 2019-10-28T20:04:13Z

It looks like anywhere there's a forward declaration your changes caused damage. At least <type_traits> line 1941 needs the C++14 swap fixed up, and the swap definition in <utility> fixed up to match. Looks like <chrono> is unhappy too.

miscco · 2019-10-29T05:43:45Z

@BillyONeal , sorry For the noise. I should have seen that I only changed one definition of swap.

I think it should be fixed together with the one from chrono.

As far as I can tell, this should only ever occur with a defaulted on named template type at the end of the template arguments. So I grepped for class> but couldnt find any other occurence. However it could be that I just missed one, because e.g. chrono gave the default type the name _Enable. However I couldnt find any other occurences of that

StephanTLavavej · 2019-10-31T03:01:02Z

/azp run

azure-pipelines · 2019-10-31T03:01:12Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-10-31T03:09:39Z

It appears that this fails to build - do you observe that locally?

stl/inc/utility

miscco · 2019-10-31T18:59:44Z

@StephanTLavavej I tested a complete rebuild in x64. that works

CaseyCarter · 2019-10-31T19:42:37Z

Grepping for =\s*enable_if|class\s*=\s*[a-uw-zA-Z0-9_] turns up one class = bool that should be ignored, and several occurrences of indirect type SFINAE (e.g., class = _Is_string_view_ish<_StringViewIsh>) that we should probably address as well.

miscco · 2019-10-31T20:21:34Z

Addressed the _Is_string_view_ish<_StringViewIsh> pattern

StephanTLavavej · 2019-10-31T21:49:55Z

/azp run

azure-pipelines · 2019-10-31T21:50:05Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-11-01T18:44:52Z

/azp run

azure-pipelines · 2019-11-01T18:45:01Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej

I've ported this to our Microsoft-internal git repo and I'm running builds and tests now. The related PR #244 encountered compiler bugs in two different front-ends, so we'll see what happens.

CaseyCarter · 2019-11-01T20:13:43Z

/azp run

azure-pipelines · 2019-11-01T20:13:51Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-11-01T20:48:40Z

/azp run

azure-pipelines · 2019-11-01T20:48:48Z

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej · 2019-11-01T23:45:25Z

FYI, I've gotten our internal tests to pass, but I'm going to have to push additional changes - reverting Casey's addition, and reverting the <hash_map> changes (all due to compiler bugs, not your fault).

This addresses microsoft#187

Add TRANSITION comments for microsoft#248 and microsoft#249, blocked by compiler bugs. Add `_Enabled` names. When we don't have `enable_if_t`, this makes the template parameter's purpose clearer. When we do have `enable_if_t` but without `= 0`, this makes it somewhat clearer that we haven't forgotten the default argument (it's simply elsewhere).

StephanTLavavej · 2019-11-02T00:23:23Z

/azp run

azure-pipelines · 2019-11-02T00:23:32Z

Azure Pipelines successfully started running 1 pipeline(s).

BillyONeal · 2019-11-02T00:41:19Z

Thanks everyone for your contributions!

StephanTLavavej · 2019-11-02T01:18:11Z

Thanks for the consistency improvement!

miscco requested a review from a team as a code owner October 28, 2019 14:12

cpplearner reviewed Oct 31, 2019

View reviewed changes

stl/inc/utility Outdated Show resolved Hide resolved

cpplearner reviewed Oct 31, 2019

View reviewed changes

stl/inc/utility Outdated Show resolved Hide resolved

miscco force-pushed the SFINAE branch 3 times, most recently from e0ae52a to fe0d9c3 Compare October 31, 2019 18:29

StephanTLavavej approved these changes Nov 1, 2019

View reviewed changes

CaseyCarter force-pushed the SFINAE branch from b97a7d0 to 2977703 Compare November 1, 2019 20:23

miscco added 2 commits November 1, 2019 17:11

Consistently use "int = 0" SFINAE

1760d30

This addresses microsoft#187

Use the pattern also for _Is_string_view_ish

7e15728

StephanTLavavej added 2 commits November 1, 2019 17:11

clang-format xstring.

f37c816

StephanTLavavej force-pushed the SFINAE branch from 2977703 to ec0608d Compare November 2, 2019 00:22

StephanTLavavej requested review from BillyONeal, CaseyCarter and barcharcraz November 2, 2019 00:26

BillyONeal approved these changes Nov 2, 2019

View reviewed changes

CaseyCarter approved these changes Nov 2, 2019

View reviewed changes

StephanTLavavej merged commit 3b0a1c9 into microsoft:master Nov 2, 2019

miscco deleted the SFINAE branch January 18, 2020 08:58

Conversation

miscco commented Oct 28, 2019

Description

Checklist

Uh oh!

BillyONeal commented Oct 28, 2019

Uh oh!

BillyONeal commented Oct 28, 2019

Uh oh!

azure-pipelines bot commented Oct 28, 2019

Uh oh!

BillyONeal commented Oct 28, 2019

Uh oh!

miscco commented Oct 29, 2019

Uh oh!

StephanTLavavej commented Oct 31, 2019

Uh oh!

azure-pipelines bot commented Oct 31, 2019

Uh oh!

StephanTLavavej commented Oct 31, 2019

Uh oh!

Uh oh!

Uh oh!

miscco commented Oct 31, 2019

Uh oh!

CaseyCarter commented Oct 31, 2019

Uh oh!

miscco commented Oct 31, 2019

Uh oh!

StephanTLavavej commented Oct 31, 2019

Uh oh!

azure-pipelines bot commented Oct 31, 2019

Uh oh!

StephanTLavavej commented Nov 1, 2019

Uh oh!

azure-pipelines bot commented Nov 1, 2019

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

CaseyCarter commented Nov 1, 2019

Uh oh!

azure-pipelines bot commented Nov 1, 2019

Uh oh!

StephanTLavavej commented Nov 1, 2019

Uh oh!

azure-pipelines bot commented Nov 1, 2019

Uh oh!

StephanTLavavej commented Nov 1, 2019

Uh oh!

StephanTLavavej commented Nov 2, 2019

Uh oh!

azure-pipelines bot commented Nov 2, 2019

Uh oh!

BillyONeal commented Nov 2, 2019

Uh oh!

StephanTLavavej commented Nov 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants