Bug 5547: assertPred (the druntime part) #41
Conversation
Also, show more digits in floating point numbers if possible.
Bug 5547: assertPred (the druntime part)
This won't work out. It creates a circular dependency between druntime and phobos. |
@dawgfoto The only way to break the dependency is to put |
return (cast(string function(T) pure nothrow @safe)&f)(value); | ||
} | ||
}; | ||
enum tango_impl = q{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is Tango code even doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexrp : Read the description of the pull request!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the runtime for D2. The Tango port for D2 works alongside Phobos. I don't see why you want to special case anything for Tango here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexrp : The Tango port for D2 works alongside Phobos does not mean Phobos must exist when Tango is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this logic, we would still have to special case every single standard library people come up with here. I don't like this. The dependency on Phobos is already very controversial IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexrp : Yes if the 3rd standard library appears we need to write code for that, otherwise it will just return T.stringof
. This has been explicitly written in the pull request description, and I am not going to change it, because this is the best solution with minimal change. You could request the repo owner to revert the commit.
Right, AFAIK we don't have any string formatting code in druntime or the compiler. In the long term a no-magic library implementation of assertPred is cleaner and extensible. |
I agree with @dawgfoto that we should avoid a dependency on Phobos. I don't really care how it's done (moving string conversion functionality to druntime or whatever), but druntime just shouldn't depend on Phobos. This is the runtime library, and it should work independently of any other D library (be it the 'standard' library or not). |
@alexrp @dawgfoto : Please notice that this do compile independently of Phobos/Tango, only that it returns
If you think Druntime forwarding to Phobos/Tango is bad, please devise a solution. I know there are other solutions, but they are no better than the current situation.
|
I understand, but I think the problem is with bootstrapping. If you're compiling the DMD tool chain on a system with no existing DMD tool chain, you're going to have to first compile druntime, then phobos, and then druntime again to get the full functionality out of this. That's how I understand it, at least. It's not the end of the world, but it does complicate a proper build.
Well, DMD has a lot of other special cases for
Probably overkill. Is all of it really needed?
This I can get behind, but I must confess to not knowing how much code would have to be moved. |
One thing that also worries me is that by using |
@alexrp :
No you don't need to do that. The "dependency" is only in the source level.
Only
I don't see any dependency on
I don't know, but it seems there are other use cases of it: http://forum.dlang.org/thread/20120323175428.GA28704@quickfur.ath.cx |
Hmm, I haven't looked at the DMD pull request yet, but I take it DMD just instantiates it then? If this is the case, what are your thoughts, @dawgfoto?
I meant the "did you mean" hint. I know how/why DMD 'depends' on some things in Phobos (directly or indirectly), but my point was that having this extra dependency probably isn't a big deal, since |
And now they're out of sync and rt.util.utf is slow while std.utf is fast. I agree that one could definitely expect more from a modern language. Some ideas for formatting:
This would only be slightly more code than __unittest_toString. Some more ideads:
|
It will get compiled into druntime's unittests and they could no longer be run without phobos. Not a single dependency on a phobos symbol must end up in the druntime library or we can no longer link programs. |
@dawgfoto :
That is equivalent to my solution number 4, without spelling out (Besides, writing some completely new code to perform something already solved, just to get rid of a dependency inside the same product? Smells like a modularization failure.)
The current implementation in DMD is already a rewrite, as described in https://github.com/D-Programming-Language/dmd/pull/263/files#L0R47.
I don't see any problem with it.
No it does not. Please read the whole commit. I have defined a |
Different product. Some smallish printf machinery would suffice.
I've seen that, it's quite nice but biased towards doing things in the compiler. You know the limitations of previous attempts (AA, Complex or Array). The problem could be nicely solved with macros, but currently it requires a mixin and stringified expressions. template Assert(string exp)
{
enum Assert = "assert("~exp~", \""~exp~"\");";
}
void main()
{
int i;
mixin(Assert!"i == 0");
mixin(Assert!"i != 0");
} |
@dawgfoto
The AA, complex and array type do not need compiler compiler support. The AA literal, complex literal, and array literal syntax do. The same in 5547 here: the assertion function do not need compiler support, but the assertion syntax do. All the
Your solution solved nothing. The point of the request is to make |
@dawgfoto And back to the formatting again,
I doubt that. It needs to support printing anything that may appear in the assertion expression, like
The effort would be like duplicating half of Someone could of course step up and write such |
That was only loud thinking. This issue pops up regularly and that we cannot solve it within the language is a deficiency. Adding tailor-made magic for assert breaks composability and language syntax rules. void assertEqual(R1, R2)(R1 r1, R2 r2)
{
for (;!r1.empty && !r2.empty; r1.popFront(), r2.popFront())
if (r1.front != r2.front)
throw new AssertError("r1 == r2");
} What am I supposed to do here? We already made the bad decision to make assert a language construct. // none of these work
auto passert = verboseTests ? &verboseAssert : &assert;
alias assert foo; Having it behave differently in unittest blocks is fairly odd when it should behave differently during unittests. This means it won't work for testing contracts. |
And again, string mapping, concatenation and formatting shouldn't be hardwired in the compiler. It's simple enough to translate that information into a template-function call. |
@dawgfoto If you have comments on dlang/dmd#263, can you please comment over there? Or put an inline note (Click the "+" speech bubble on the left of the line)? This pull request is about Druntime and it has been closed, if we keep talking here, there will likely be almost no one noticing these opinions except us.
That's not loud thinking, that's a fact. Your solution in #41 (comment) solved nothing. Who cares about the message "AssertError: i == 5", we already know the file and line number and the stack trace. What is valuable is the value of
Not sure what issue do you mean. If you mean AST transformation, I agree totally.
Write the code properly using void assertEqual(R1, R2)(R1 r1, R2 r2)
{
for (;!r1.empty && !r2.empty; r1.popFront(), r2.popFront())
assert(r1.front == r2.front);
} If you want to throw an AssertError you are on your own. (Just like when you use the GC function directly, or an ASM block... if you use low-level construct, don't expect the compiler to help you.)
It can be changed to work in testing contracts. (But apparently contracts is such a failure that no one adds this requirement in 5547 ^^) |
This was introduced as part of GitHub pull request dlang#41, which was questionable (!) support code for dlang/dmd#263. However, the latter never materialized and __unittest_toString plus version(druntime_unittest) were just confusing oddities at this point.
MSVCx86: use pretty names instead of mangled names for exceptions
See dlang/dmd#263.
Important: The pull request dlang/dmd#725 should be merged before pulling this.
Just provides the function
__unittest_toString
, which basically forwards tostd.conv.to!string
ortango.utils.Convert.to!string
. If there is a third standard library (gasp) the corresponding code have to be added manually.(BTW, is there a good way to decouple Phobos/Tango without rewriting
to!string
in druntime?)requires: dmd git://github.com/kennytm/dmd.git bug7399-import-too-fatal