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.

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

Make all toImpl overloads private #4208

Merged
merged 3 commits into from
Apr 28, 2016
Merged

Conversation

JackStouffer
Copy link
Member

@JackStouffer JackStouffer changed the title Made all toImpl overloads private Make all toImpl overloads private Apr 17, 2016
@JackStouffer JackStouffer force-pushed the toImpl branch 2 times, most recently from d0dbe2f to eb13d61 Compare April 17, 2016 22:34
@ghost
Copy link

ghost commented Apr 17, 2016

after a check the documentation generated becomes more confusing:

  • template to(T) , the to function...: what ? function ? template ?
  • from type Source to type Target: it's a template with a single parameter named T.

one solution is to write the doc in a version(D_Ddoc){} block, as it is already done here and there in phobos.

version(D_Ddoc)
{
    /// the ddoc and the declaration used to generate the doc. 
    Target to(Target, Source)(Source source);
}
else
{
    // the real content
}

@schuetzm
Copy link
Contributor

There are now lots of instances of "to" that need an "_" added, because Ddoc makes them bold otherwise.

Also, not directly related to this PR, but IMO the examples in the documentation would profit from UFCS to make them more readable.

@JackStouffer
Copy link
Member Author

@bbasile @schuetzm Addressed comments

@JackStouffer
Copy link
Member Author

@MartinNowak Is this breakage acceptable?

@ghost
Copy link

ghost commented Apr 18, 2016

Thanks for listening although I would have prefered the cheat indicated earlier with D_Ddoc. Anyway ok now.

Another thing: there was an old enhancement request that was closed recently (handling of hex prefix in this function). Could you add a note here, after the grammar for the unsigned numbers:

For conversion to unsigned types, the grammar recognized is:

    UnsignedInteger:
       DecimalDigit
       DecimalDigit UnsignedInteger

Something like

Note that the specific syntax used for D hexadecimal and binary literals is 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.

@JackStouffer
Copy link
Member Author

@bbasile Can you post a link to that bug so I can link to it in the docs?

@ghost
Copy link

ghost commented Apr 18, 2016

It's not a bug: https://issues.dlang.org/show_bug.cgi?id=9797.

It's something that won't be done, so the ddoc must specifies that certain forms of literals are not accepted. Thx BTW, I was too lazy to do it myself...

@JackStouffer
Copy link
Member Author

@bbasile I added a note with an example

* `to!(double[])` applies _to an `int[]`. The conversion might throw an
* exception because `to!short` might fail the range check.
*/
pure unittest
Copy link

Choose a reason for hiding this comment

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

pure seems to be the origin of the latest auto tester failure. Old UT for AA at line 1096 was not pure !

@DmitryOlshansky
Copy link
Member

LGTM

@ghost
Copy link

ghost commented Apr 20, 2016

LGTM ✔️

@JackStouffer
Copy link
Member Author

I take back my previous request. If @MartinNowak doesn't like it he can always revert it before the release.

@wilzbach
Copy link
Member

LGTM^3 - maybe we should add a small warning to the changelog?

@JackStouffer JackStouffer force-pushed the toImpl branch 2 times, most recently from 2aecaa9 to d259c40 Compare April 25, 2016 20:28
@JackStouffer
Copy link
Member Author

@wilzbach done

@wilzbach
Copy link
Member

This branch has conflicts that must be resolved

I assume the changelog file?
Yet another reason for #4228 ;-)

@JackStouffer
Copy link
Member Author

rebased

@JackStouffer
Copy link
Member Author

ping @DmitryOlshansky I think this is good to go

@JackStouffer
Copy link
Member Author

@schuetzm I think I caught them all now

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants