std.numeric: clean import & CustomFloat printing fix #2729
Conversation
| static assert(__traits(compiles, findRoot!real((x)=>cast(double)x, real.init, real.init))); | ||
| static assert(__traits(compiles, findRoot((real x)=>cast(double)x, real.init, real.init))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Oh nevermind, I see that you just moved it.
|
LGTM |
|
Auto-merge toggled on |
| @@ -619,7 +617,16 @@ public: | |||
| } | |||
|
|
|||
| /// ditto | |||
| string toString() { return to!string(get!real); } | |||
| template toString() | |||
There was a problem hiding this comment.
Why is toString a template nested inside a template? It almost certainly isn't worth complicating the code like this just to maybe avoid one single import from happening that is already inside a template instance. Moreover, both of the imports are for std.format. Thus, you only gain something once the compiler implements lazy semantic analysis. But then, the whole thing will probably superfluous superfluous anyway, with CustomFloat being a template. Also note that template instances aren't free either.
There was a problem hiding this comment.
CustomFloatcan be used without printing it. Sotemplate toString()is useful to do not importstd.format.void toString()(scop ...compiles andvoid toString(scop ...not compiles with unittests! I don't know why.
There was a problem hiding this comment.
The correct thing to do for 2. is not to randomly add parentheses and then hope that the problem goes away. If this turns out to be a genuine compiler bug, there should at the very least be a comment explaining why the code looks so messy.
There was a problem hiding this comment.
OK. Do we need reduced example?
|
Auto-merge toggled off |
1 similar comment
|
Auto-merge toggled off |
| template toString() | ||
| { | ||
| import std.format : FormatSpec; | ||
| void toString()(scope void delegate(const(char)[]) sink, FormatSpec!char fmt) |
There was a problem hiding this comment.
Yeah, just noticed the extraneous () here. If this is already inside a template block, toString shouldn't take any compile-time parameters, since toString()(...) is the same as template toString() { toString(...) ...}.
There was a problem hiding this comment.
Without extra () code doesn't compile!
There was a problem hiding this comment.
Unittes fails. std.format can not understand that this struct has toString.
There was a problem hiding this comment.
core.exception.AssertError@/Users/ilya/GitHub/phobos/std/numeric.d(671): unittest failure
There was a problem hiding this comment.
To do not compile it and import std.format when toString is unused.
There was a problem hiding this comment.
I see. In that case, we need to trace down the problem of why std.format doesn't recognize toString when it's inside an explicit eponymous template (rather than an implicit one that we usually use).
There was a problem hiding this comment.
ping @klickverbot @quickfur
Reduced example:
module test2155a;
struct B {}template A()
{
import test2155a : B;
void A(B b)
{
import std.format;
}
}
void foo() {
import test2155a : B;
B b;
A(b);
}test2155.d(4): Error: undefined identifier B
There was a problem hiding this comment.
Sounds like a compiler bug / limitation.
There was a problem hiding this comment.
|
@quickfur can we merge with comment?: template toString()
{
import std.format : FormatSpec, formatValue;
// FIXME "()", Issue 13737
void toString()(scope void delegate(const(char)[]) sink, FormatSpec!char fmt)
{
sink.formatValue(get!real, fmt);
}
} |
|
Thanks! This makes me much more confident that there isn't something else we might have missed that is fundamentally broken. In Phobos, |
update toString template Issue 13737 new comment add brace
|
Auto-merge toggled on |
|
Thanks! |
std.numeric: clean import & CustomFloat printing fix
Blocker for #2728