Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consistently use "int = 0" SFINAE #226

Merged
merged 4 commits into from
Nov 2, 2019
Merged

Consistently use "int = 0" SFINAE #226

merged 4 commits into from
Nov 2, 2019

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Oct 28, 2019

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.

@miscco miscco requested a review from a team as a code owner October 28, 2019 14:12
@BillyONeal
Copy link
Member

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

@BillyONeal
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

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
Copy link
Contributor Author

miscco commented Oct 29, 2019

@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
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

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

cpplearner
cpplearner reviewed Oct 31, 2019
View reviewed changes
stl/inc/utility Outdated Show resolved Hide resolved
cpplearner
cpplearner reviewed Oct 31, 2019
View reviewed changes
stl/inc/utility Outdated Show resolved Hide resolved
@miscco miscco force-pushed the SFINAE branch 3 times, most recently from e0ae52a to fe0d9c3 Compare October 31, 2019 18:29
@miscco
Copy link
Contributor Author

miscco commented Oct 31, 2019

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

@CaseyCarter
Copy link
Member

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
Copy link
Contributor Author

miscco commented Oct 31, 2019

Addressed the _Is_string_view_ish<_StringViewIsh> pattern

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

StephanTLavavej
StephanTLavavej approved these changes Nov 1, 2019
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephanTLavavej
Copy link
Member

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).

@StephanTLavavej
clang-format xstring.
f37c816
@StephanTLavavej
Change duration_cast()'s definition to fix the build.
ec0608d 
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
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

Thanks everyone for your contributions!

@StephanTLavavej StephanTLavavej merged commit 3b0a1c9 into microsoft:master Nov 2, 2019
@StephanTLavavej
Copy link
Member

Thanks for the consistency improvement!

@miscco miscco deleted the SFINAE branch January 18, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpplearner cpplearner cpplearner left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@BillyONeal BillyONeal BillyONeal approved these changes

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@barcharcraz barcharcraz Awaiting requested review from barcharcraz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@miscco @BillyONeal @StephanTLavavej @CaseyCarter @cpplearner