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

[trivial] std.conv: Don't allow to!T() in constraint #4898

Merged
merged 4 commits into from Nov 23, 2016
Merged

[trivial] std.conv: Don't allow to!T() in constraint #4898

merged 4 commits into from Nov 23, 2016

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Nov 12, 2016 •
edited

Also document to template functions within parent to template.

@ntrel
Fix Issue 16478 - Don't allow to!T() in constraint
006ee44 
Also avoid highlighting 'to' unnecessarily.
@ntrel
Remove isRawStaticArray - same as isStaticArray
b967d94 
isStaticArray!T already checks !isAggregateType!T.
@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
✓ 16478 Don't allow to!T() in constraint

ntrel
ntrel commented Nov 12, 2016
View reviewed changes
std/conv.d
T to(A...)(A args)
if (!isRawStaticArray!A)
if (A.length > 0 && !isStaticArray!(A[0]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we should have 2 overloads instead of just using args here, I can do that after this pull is merged.

ghost
ghost reviewed Nov 12, 2016
View reviewed changes
std/conv.d
enum isRawStaticArray =
A.length == 0 &&
isStaticArray!T &&
!is(T == class) &&
Copy link

@ghost ghost Nov 12, 2016 •
edited by ghost

Choose a reason for hiding this comment

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

These static tests are for alias <member that's a static array> this, can they really be suppressed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, please see the full commit message.

Copy link

Choose a reason for hiding this comment

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

OK.

ghost
ghost reviewed Nov 12, 2016
View reviewed changes
std/conv.d
@@ -230,7 +221,7 @@ template to(T)
* When converting strings _to numeric types, note that the D hexadecimal and binary
* literals are not handled. Neither the prefixes that indicate the base, nor the
* horizontal bar used _to separate groups of digits are recognized. This also
* applies to the suffixes that indicate the type.
* applies _to the suffixes that indicate the type.
Copy link

Choose a reason for hiding this comment

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

don't merge two fixes. the to -> _to is another issue. If your PR gets merged for the code and reverted because of regression than the fix for the DDOC will also be lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, yes. But I think this is what most people do for very minor changes, otherwise we'd get tons of PRs.

Copy link

Choose a reason for hiding this comment

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

You hit a wall here. Don't merge two things in one PR. End of story. Waiting for you to amend.
Open another PR for the ddoc.

Copy link

Choose a reason for hiding this comment

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

we'd get tons of PRs.

That's not a problem, annotate it with [TRIVIAL, DDOC] and it'll be merged quickly.

Copy link
Contributor Author

@ntrel ntrel Nov 14, 2016 •
edited

Choose a reason for hiding this comment

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

Removed doc changes.

@ntrel ntrel mentioned this pull request Nov 14, 2016
Closed
@ntrel
Copy link
Contributor Author

ntrel commented Nov 23, 2016

@bbasile Merge please?

schveiguy
schveiguy reviewed Nov 23, 2016
View reviewed changes
std/conv.d
@@ -178,14 +167,14 @@ $(I UnsignedInteger):
template to(T)
{
T to(A...)(A args)
if (!isRawStaticArray!A)
if (A.length > 0 && !isStaticArray!(A[0]))
Copy link
Member

Choose a reason for hiding this comment

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

I believe the previous constraint is equivalent to this instead:

if(A.length > 0 || !isStaticArray!(A[0]))

The calls it allows vs. your constraint, I'm not sure are valid. But this is a difference.

Copy link
Member

@schveiguy schveiguy Nov 23, 2016 •
edited

Choose a reason for hiding this comment

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

Boy, that's not a very good constraint :)

This is better

if(A.length != 1 || !isStaticArray!(A[0]))

@schveiguy
Copy link
Member

LGTM, just fix the constraint, and I will merge.

schveiguy
schveiguy approved these changes Nov 23, 2016
View reviewed changes
Copy link
Member

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

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

Nevermind, I forgot I can just fix it myself :)

@schveiguy
Copy link
Member

Auto-merge toggled on

@schveiguy schveiguy merged commit 6a059e7 into dlang:master Nov 23, 2016
@ntrel
Copy link
Contributor Author

ntrel commented Nov 24, 2016

@schveiguy: You have undone the fix for the issue.

if (A.length != 1 || !isStaticArray!(A[0])) - this allows to!string() to compile!

I think we can use specialization to achieve what you want neatly:

    T to(A...)(A args)
        if (A.length > 0);
    T to(S)(ref S arg)
        if (isStaticArray!S);

Note: my original code was actually correct because for more than one argument, the only options are for the first argument to pass isIntegral or to pass isInputRange, neither of which apply for static arrays.

@ntrel ntrel mentioned this pull request Nov 24, 2016
Merged
@schveiguy
Copy link
Member

I thought the issue was that the internal isRawStaticArray appears in the error message?

@schveiguy
Copy link
Member

to!string() shouldn't compile, and I don't think it did with the original code, right? So why change the constraint?

@ntrel
Copy link
Contributor Author

ntrel commented Nov 24, 2016

You're right it doesn't successfully compile, but the constraint should prevent internal errors with toImpl. Fix in #4922.

@ntrel
Copy link
Contributor Author

ntrel commented Nov 24, 2016

BTW Thanks for reviewing, the original issue is much better after this merge ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schveiguy schveiguy schveiguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ntrel @dlang-bot @schveiguy