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

Issue 4287 - Implement concat-assign operator for Appender. #1108

Merged
merged 2 commits into from Feb 7, 2013
Merged

Issue 4287 - Implement concat-assign operator for Appender. #1108

merged 2 commits into from Feb 7, 2013

Conversation

ghost
Copy link

@ghost ghost commented Feb 3, 2013

@DmitryOlshansky
Copy link
Member

Removeing trailing whitespace change and other no-changes go as separate commit.
Otherwise LGTM

private template CanPutItem(U)
{
enum bool CanPutItem = isImplicitlyConvertible!(U, T) ||
isSomeChar!T && isSomeChar!U;
Copy link
Member

Choose a reason for hiding this comment

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

Parenthesise this as || and && have the same priorty IRC. And I think it's bogus w/o them.
Also how about add a test for appender of ints explicitly?

EDIT: checked the online spec, to my shame I've found out && has higher priority and thus evaluted first, so this is a not an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Prolly you thought about & and |, which indeed have the same priority.

Copy link
Author

Choose a reason for hiding this comment

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

Parenthesise this as || and && have the same priorty IRC. And I think it's bogus w/o them.

The code is a cut&paste of existing template constraints. All I did was put them in a template so I can use them from both the put and opOpAssign functions to avoid code duplication.

Also how about add a test for appender of ints explicitly?

I didn't change any semantics, the operators simply forward to put functions.

@ghost
Copy link
Author

ghost commented Feb 4, 2013

Removing trailing whitespace change and other no-changes go as separate commit.

This should be part of detab+toLF+strip functionality which was already discussed about. I really can't waste any of my time trying to figure out if a file is in a proper state before I start editing it, the trailing whitespace should have been caught by reviewers.

@@ -2219,11 +2219,28 @@ Returns the managed array.
return newext > newlength ? newext : newlength;
}

private template CanPutItem(U)
Copy link
Member

Choose a reason for hiding this comment

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

canPutItem since it's a value, not a type.

@ghost
Copy link
Author

ghost commented Feb 7, 2013

Separated detab to its own commit and changed casing for the templates.

@quickfur
Copy link
Member

quickfur commented Feb 7, 2013

LGTM.

alexrp added a commit that referenced this pull request Feb 7, 2013
Issue 4287 - Implement concat-assign operator for Appender.
@alexrp alexrp merged commit 2a88fea into dlang:master Feb 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants