-
-
Notifications
You must be signed in to change notification settings - Fork 701
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
Fix issue 19003: don't call toString on T.init #6594
Fix issue 19003: don't call toString on T.init #6594
Conversation
Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#6594" |
f134628
to
1f801df
Compare
ping |
1 similar comment
ping |
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.
Looks good. I'd put the ThenStatement
s on their own lines but since CircleCI is green i suppose it's good.
I didn't want the change to become too verbose; this way it's structurally easy to parse. I don't know how else to do it, given that the final else should still always run. |
std/format.d
Outdated
|
||
invariant { assert(this.i); } | ||
|
||
this(int i) @safe in { assert(i); } body { this.i = i; } |
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.
s/body/do/
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.
fixed
1f801df
to
c10999e
Compare
ping |
Perhaps because someone is calling it for a side-effect (like timing a formatting operation) or because someone is calling it to make sure that the code runs. Neither of those happen if the code is blocked out with |
It seemed kind of redundant to implement a DontCallToStringSink { NullSink ns; alias ns this; }, but sure. The important thing is I really don't want to add a template parameter and hunt it all the way through the toStrings, especially since it's a public API. |
It's not obvious at a cursory glance: can you explain why the "invalid format" check still works with the code behind |
The "invalid format" check has largely nothing to do with toString(); rather, it checks for the format string being valid, whose parsing is done by formattedWrite and friends. However, like the Nullable issue but more so, this relies on T.init being a valid value that you can call methods on. (I'm increasingly becoming convinced that the case of structs with invalid T.init was never intended - so I really don't understand why you can |
c10999e
to
d6c41a3
Compare
@n8sh Replaced |
d6c41a3
to
be8dace
Compare
oops, tiny fix - readded a comment I accidentally removed |
@n8sh note that it won't merge despite that label due to the lack of approving review. What do? |
Well there is also a CircleCI failure |
Yeah, that sucks for CircleCI. It has nothing to do with this PR though. edit: Oh you mean it won't merge cause a required check is failing? Well that sucks. It just says "review required" though and "merging can be performed with one approving review." |
A rebase would help then
Yes, and people are less eager to review a PR which is red. |
Would it? It's been red everywhere for about a week at this point. edit: fine, let's try again edit: it failed again, oh no |
…to avoid toString actually being called.
be8dace
to
902468f
Compare
No need to waste resources on this. See #ci or check #6626 |
Merged the CircleCi PR and restarted it. It should pass now. Jenkins is still a PITA, but we're planning to switch to Buildkite. |
yay! |
Avoid actually calling
toString
when speculatively formatting to check for format exceptions.Args.init
is not a constructed value; hence it is not safe to call methods on it.Two-step solution:
avoid actually calling
toString
when writing intoNullSink
(why bother?)call
formattedWrite
to write intoNullSink
incheckFormatException
.