Tuple's toString made useful by Bolpat · Pull Request #3594 · dlang/phobos

Bolpat · 2015-08-27T22:00:22Z

From http://forum.dlang.org/thread/srofuqyrdzphgzylbwds@forum.dlang.org
With this, you can easily format a Tuple from its content, similar how to do with an array.
The toString(delegate, FormatSpec) function takes the formats %(nested%) and %(nested%|separator%) a bit different: The second one treats the Tuple as it was an array. The first one applies the nested format to the expanded fields, like format(nested, tup.expand) did.
With this, the question http://forum.dlang.org/post/ntqawkupddjcijwtwovp@forum.dlang.org gets a trivial answer: Use "%(%(%s%| %)\n%)" insted of "%(%(%s %)\n%)".
It can be expected not to break any existing code.

JakobOvrum · 2015-08-27T23:14:09Z

std/typecons.d

Just do is(Type == shared)?

Just do is(Type == shared)?

I had that in mind, too, but I've never dealt with shared types. Indeed, I believe there is a difference that is not obvious, just because those who wrote that should have had this in mind—but didn't do that. Therefore I left it like that.
In the next pull request, it'll be the simple one. Let's see what others think about it.

Bolpat · 2015-08-31T00:35:04Z

I'm new to this. @JakobOvrum can you (or someone else) explain me why the auto-tester rejects my code? I'm pretty sure that error: servname not supported for ai_socktype are not my fault.

schveiguy · 2015-08-31T22:59:19Z

@Bolpat Tests that infrequently fail can make their way into the auto tester. It's annoying, because it seems like your change is being rejected, but it's just complete dumb luck.

However, your pull is failing pretty much everything. That doesn't necessarily mean the code you have written is wrong, it may be that it uncovered a latent bug. If you can run unit tests locally and get a failure, it may help.

Bolpat · 2015-09-01T03:19:56Z

@schveiguy Thanks! I've tried like this: Create a new file with only main, only containing the unit test. It worked perfectly. Then I tried rdmd -main -unittest typecons.d. It failed due to an seg. fault. I couldn't figure out where it occurs, but I can tell that even the old (unmodified) copy of typecons.d failed this test. Maybe I've forgotten something to tell the compiler?

CyberShadow · 2015-09-01T15:13:26Z

I'm new to this. @JakobOvrum can you (or someone else) explain me why the auto-tester rejects my code? I'm pretty sure that error: servname not supported for ai_socktype are not my fault.

That is an unrelated soft failure. Ignore it. The real failure is somewhere else in the same log. For example now I see:

gmake[1]: *** [unittest/std/typecons.run] Illegal instruction: 4 (core dumped)

Maybe this?

Bolpat · 2015-09-01T18:53:15Z

I now used Phobos' unittest.d. It works with the submitted unit test, and fails on a altered one supposed to fail.
@CyberShadow Thank you for the hint. I'll investigate this.

JakobOvrum · 2015-10-26T13:55:57Z

std/typecons.d

The brackets around fmt.spec shouldn't be needed; char should be able to be concatenated with slices of char freely. It is an unnecessary GC allocation.

JakobOvrum · 2015-10-26T14:05:41Z

I'm sorry for this hanging so long, this is a really cool feature that we should get merged and espouse in the changelog.

Does anyone know what's going on with the unit tester?

Bolpat · 2015-10-26T22:44:52Z

There are some strange things going on here. It seems formattedWrite doesn't work how I'd expect. If I comment the lines in (and e.g. set some debug writeln( ) before), then it clearly doesn't run over it, yet it yields SIGSEGV.

JakobOvrum · 2015-10-30T14:22:41Z

@Bolpat, I filed a pull request to your PR fixing the segfault.

Bolpat · 2015-10-31T00:15:17Z

@JakobOvrum Thanks.

JakobOvrum · 2015-10-31T05:05:02Z

std/typecons.d

This line looks redundant (and there's some strange whitespace in the line above)

It is redundant. I've deleted it, but it's not worth pushing and letting the auto-tester do it's work all again.
The real question is: Do we really want to have assertThrown in the unit test?

I think it's definitely good to test, but I'm not sure whether it's worth including in the documented unit test.

However, Tuple's unit tests might be or become a performance issue; since they are templated, there are as many copies of the unittest as there are unique template instantiations. In std.container we reverted to using DDoc example sections and moving the unittests outside the template because of this issue, despite the source code duplication it causes. That sort of change is beyond the scope of this PR, but I thought I might mention it anyway.

It's good to be mentioned. If it really is a performance issue, it has to be done. It shouldn't be a big thing, or was it in std.container?

JakobOvrum · 2015-10-31T05:08:45Z

Otherwise LGTM. Please squash the intermediate commits that deal with testing or commenting of code (and the merge commit).

Bolpat · 2015-11-04T23:54:27Z

I have a question about line 784, the alternative function declaration. Usually the character type is one of char, wchar and dchar. As phobos is a standard library, it should support all of them. When I try to use the alternative declaration, the compiler probably can't infer the char type, and the function is not used by format because hasToString evaluates to 0 instead of 4 (see template in format.d line 2770).
How can we infer Char in a neat way? I have a solution (Bolpat@d708ea4) but I'm not really convinced of it. More or less, it's brute forcing it.

JakobOvrum · 2015-11-05T12:25:01Z

The commented out signature should work just fine:

void toString(DG, Char)(scope DG sink, std.format.FormatSpec!Char fmt = "%s") const

Bolpat · 2015-11-05T23:17:02Z

No, it definitely doesn't. Not like this and the two thousand other ways I tried. Is it even worth?

JakobOvrum · 2015-11-06T09:19:21Z

It works for me when I remove the full qualification of std.format.FormatSpec, to just FormatSpec. I don't know why that's the case.

edit:

The default argument doesn't make any sense for FormatSpec!Char, though; the compiler can't infer the type of Char from the default argument (for good reasons). If the default is still desirable, use overloading instead.

Bolpat · 2015-11-06T10:24:33Z

When I wrote the full qualification, I did it because it was necessary. So you've imported FormatSpec before, right?
In the alternative branch (FormatSpec) to investigate that, I've come to the conclusion that the default doesn't do what I expect it to, so it should be discarded.
There is still a lot to do...

JakobOvrum · 2015-11-06T10:43:50Z

Yes, std.format needs to be imported. You can put the import declaration inside the toString template.

I've come to the conclusion that the default doesn't do what I expect it to

And what exactly do you expect it to do?

Bolpat · 2015-11-06T13:44:14Z

Yes, std.format needs to be imported. You can put the import declaration inside the toString template.

That looks so odd to me (from my C++ perspective) so I indeed didn't try.

I've come to the conclusion that the default doesn't do what I expect it to

And what exactly do you expect it to do?

I thought the constructor would parse the input. It just passes it to trailing which has no use in this case. Nothing to mention.

JakobOvrum · 2015-11-14T01:04:02Z

@Bolpat, if the generalisation of FormatSpec is what's blocking progress, please just leave it to a future PR; it would be a backwards-compatible change.

edit:

Oh, you've fixed it, nevermind.

JakobOvrum · 2015-11-14T01:06:18Z

std/typecons.d

This needs at least an empty documentation comment (e.g. ///) for the documentation for the nested symbols to show up.

Bolpat · 2015-11-15T19:05:14Z

There are some things I'm suspicious about. In format.d there is the hasToString(T, Char) template which generalizes the FormatSpec parameter of the toSting method to be tested, but it does not generalize the character type of the delegate. Is that intended? At this point hasToString(Tuple, char) == 4, but hasToString(Tuple, Char) == 2 otherwise.

Any conclusion on function attributes?

It seems toString() is const pure @safe by compiler's inference. So all other overloads should (at least) have these. I got that from an error message saying need 'this' for 'toString' of type 'const pure @safe string()' while evaluating pragma(msg, __traits(getFunctionAttributes, toString()()))
I hope that was what you wanted to know. Should I add them?

Bolpat · 2015-11-16T02:18:57Z

Another question, now that Tuples have first-class toString, is it still appropriate to have the %s format produce the improvised style Tuple!( types )( values )? As a newcomer I'd expect it to be just the ( values ). But changing this behavior would definitely break lots of unit tests, doesn't it. I've included some lines to do that, but they're commented out.

JakobOvrum · 2015-11-17T02:35:02Z

The attributes should be added to the unittest, not toString. Also note that const is not a function attribute but a type qualifier, so it should remain on toString.

As for templatizing the chunk type or changing the textual representation of "%s" for Tuple, please leave it to a future PR.

Bolpat · 2015-11-18T23:58:36Z

Some Results: The unittest is @safe. If the component types of the Tuple provide @safe operations, then toString seems to be too. Poor toSting is impure because it calls formatValue calling _snprintf which is not compile-time executable. The compiler cannot verify pureness.

JakobOvrum · 2015-11-23T03:16:17Z

LGTM. Please clean up the commits so we can merge.

Bolpat · 2015-11-23T18:57:39Z

What do you exactly mean with cleaning up? Squash all commits?

JackStouffer · 2015-11-23T19:11:24Z

@Bolpat Yes, this should be one commit.

dlang-bot · 2015-11-23T20:04:55Z

Fix	Bugzilla	Description
✓	13324	dynamically load libcurl at runtime
✓	14605	RefAppender fails isOutputRange
✓	14884	`among` docs broken link to `find` and `canFind`
✓	14949	Non-descriptive "Enforcement failed" when attempting to write to closed file

Bolpat · 2015-11-23T20:05:31Z

What's going on here?

JackStouffer · 2015-11-23T20:21:07Z

Oh boy.

I'm assume you tried to rebase on master right? Try this

git rebase -i HEAD~41

This will bring you into an interactive session in your preferred command line text editor. Use the on screen instructions to delete the commits that aren't yours and squish the ones that are yours (d and s respectively).

JackStouffer · 2015-11-23T20:24:27Z

If that worked, you'll have to git push -f in order to force git to accept you rewriting history.

# The first commit's message is: # This is a combination of 10 commits. # The first commit's message is: # This is a combination of 2 commits. # The first commit's message is: # This is a combination of 5 commits. # The first commit's message is: Tuple's toString made useful. http://forum.dlang.org/thread/srofuqyrdzphgzylbwds@forum.dlang.org Format Tuple's fields with format in %(%|%), similar to arrays, or, without %|, each filed specifically. Doesn't break any existing code as %s qualifier's behavior is not modified The overload without parameters is intentionally left as a template, as it appears to reveal some bugs in std.format for certain types. Remove the template parameter list to reproduce. # This is the 2nd commit message: dynamically load libcurl - avoids issues with versioned symbols on different platforms - lazy loading/initialization of curl library - fix Issue 13324 - try to load curl from the exe itself before loading a shared library thus allowing to link against a different or a static curl library # This is the 3rd commit message: fix Issue 14949 - Non-descriptive "Enforcement failed" when attempting to write to closed file # This is the 4th commit message: remove data argument from options - it's unlikely that the non-usage of OPTIONS body changes anytime soon # This is the 5th commit message: Allow Variant.visit to support const types. Assuming that Foo.depth and Bar.depth are both const (e.g. both are just int members), the following should compile: ----- int depth(in FooBar fb) { return fb.visit!((Foo foo) => foo.depth, (Bar bar) => bar.depth); } ----- However, it was failing with: std/variant.d(2246): Error: cannot implicitly convert expression (variant.peek()) of type const(Foo)* to Foo* std/variant.d(2246): Error: cannot implicitly convert expression (variant.peek()) of type const(Bar)* to Bar* This patch changes an explicit `T*` declaration to an `auto` declaration so const types can be visited. # This is the 2nd commit message: fix docs for HTTP only requests # This is the 2nd commit message: Fix broken hyperlinks to std.algorithm submodules. # This is the 3rd commit message: Leading rows in Container Primitives table should span all columns. Depends on: dlang/dlang.org#1071 # This is the 4th commit message: Add missing references to std.digest.hmac to fix doc generation # This is the 5th commit message: Added Andrei's Getting Started Guide # This is the 6th commit message: Fix Issue 14884 # This is the 7th commit message: Ignore .lst files # This is the 8th commit message: Add sameHead/sameTail to function list in docs. # This is the 9th commit message: std.traits.hasMember: just forward to __traits(hasMember, ...) This way opDispatch'ed members are recognized. Fixes issue 14605 - RefAppender fails isOutputRange. # This is the 10th commit message: Ddoc: add missing parenthesis # This is the 2nd commit message: Added /// # This is the 3rd commit message: Added attributes; clearify Documentation

Bolpat · 2015-11-23T20:46:36Z

I'm making things worse because I'm not really knowing what to do. It prompts me with error: could not apply <some sha>. I skipped them.

JackStouffer · 2015-11-23T21:30:18Z

Well, without really being there or knowing exactly what you did, I can't troubleshoot this. Does anyone else have any ideas.

Also, I was afraid this might happen, so I made a copy of your changes before you tried rebasing.

Bolpat · 2015-11-23T21:32:32Z

@JackStouffer Thank you very, very much. I gave you and @JakobOvrum write permissions. So you can do what you think is right. That's much better than delegating me.

JackStouffer · 2015-11-25T16:06:08Z

I don't have the time to fix this myself. Here is the backup I made http://dpaste.dzfl.pl/4c8ae5692fca

I would suggest opening a new PR with the above code. Just make a new branch and delete this one.

Bolpat · 2015-11-25T18:20:23Z

The new pull request is here. In the files I've only modified some bad English in the documentation.

JakobOvrum reviewed Aug 27, 2015
View reviewed changes

JakobOvrum reviewed Oct 26, 2015
View reviewed changes

JakobOvrum reviewed Oct 31, 2015
View reviewed changes

JakobOvrum reviewed Nov 14, 2015
View reviewed changes

JackStouffer mentioned this pull request Nov 25, 2015

Update contributing guide #3832

Merged

Bolpat closed this Nov 25, 2015

JakobOvrum mentioned this pull request Jan 7, 2016

Tuple range interface #3198

Closed

Uh oh!

Conversation

Bolpat commented Aug 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bolpat commented Aug 31, 2015

Uh oh!

schveiguy commented Aug 31, 2015

Uh oh!

Bolpat commented Sep 1, 2015

Uh oh!

CyberShadow commented Sep 1, 2015

Uh oh!

Bolpat commented Sep 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Oct 26, 2015

Uh oh!

Bolpat commented Oct 26, 2015

Uh oh!

JakobOvrum commented Oct 30, 2015

Uh oh!

Bolpat commented Oct 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Oct 31, 2015

Uh oh!

Bolpat commented Nov 4, 2015

Uh oh!

JakobOvrum commented Nov 5, 2015

Uh oh!

Bolpat commented Nov 5, 2015

Uh oh!

JakobOvrum commented Nov 6, 2015

Uh oh!

Bolpat commented Nov 6, 2015

Uh oh!

JakobOvrum commented Nov 6, 2015

Uh oh!

Bolpat commented Nov 6, 2015

Uh oh!

JakobOvrum commented Nov 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bolpat commented Nov 15, 2015

Uh oh!

Bolpat commented Nov 16, 2015

Uh oh!

JakobOvrum commented Nov 17, 2015

Uh oh!

Bolpat commented Nov 18, 2015

Uh oh!

JakobOvrum commented Nov 23, 2015

Uh oh!

Bolpat commented Nov 23, 2015

Uh oh!

JackStouffer commented Nov 23, 2015

Uh oh!

dlang-bot commented Nov 23, 2015

Uh oh!

Bolpat commented Nov 23, 2015

Uh oh!

JackStouffer commented Nov 23, 2015

Uh oh!

JackStouffer commented Nov 23, 2015

Uh oh!

Bolpat commented Nov 23, 2015

Uh oh!