Skip to content

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Feb 13, 2014

@ghost
Copy link

ghost commented Feb 13, 2014

Thanks for the fix. LGTM.

src/lexer.c Outdated
Copy link

Choose a reason for hiding this comment

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

Another comma operator found here. Note that I'm mentioning this since it might eventually become an error in D, and we're heading towards DDMD anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed.

Honestly I have some negative opinion against disallowing comma operators. I doubt the necessity of the disallowing.

Copy link

Choose a reason for hiding this comment

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

I doubt the necessity of their initial inclusion to any language. What's the point except to confuse programmers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why comma operator confuse programmers? I don't think so.

Choose a reason for hiding this comment

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

It may not confuse as-is but it should definitely be reused for something more useful ;)

Copy link

Choose a reason for hiding this comment

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

Why comma operator confuse programmers? I don't think so.

Because I have to look up the spec to see how it's implemented. Specifically the order of operations. And then I have to wonder whether the compiler follows the spec.

At the end of the day you could have just used ;, but wanted to hide two statements into one just to save on bracket count.

Also, wasn't there discussion about how tuple implementations require removing the comma operator?

Copy link

Choose a reason for hiding this comment

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

Think of the comma operator as the is expression. It's a useful feature, but even advanced users may end up having to look up the documentation to remind themselves of its tricky semantics. Unless of course those experts practically implemented the feature. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I have some negative opinion against disallowing comma operators. I doubt the necessity of the disallowing.

Spot the bug:

foreach(ch; chain(iota('a', 'я'+1)), iota('А','Я'+1))

Sure, foreach could take an AssignExp to prevent this, but there are very few legitimate uses that aren't clearer without the comma.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think of the comma operator as the is expression. It's a useful feature, but even advanced users may end up having to look up the documentation to remind themselves of its tricky semantics. Unless of course those experts practically implemented the feature. :)

The is expression is a lot more complex than the comma operator. The comma operator has the same precedence as the comma in function calls, so the problem is usually forgetting it exists rather than now it works.

The is expression also cannot be trivially rewritten in terms of other constructs. Comma can be: e1, e2 -> { e1; return e2; }().

@yebblies
Copy link
Contributor

Aaaaanyway LGTM.

@yebblies
Copy link
Contributor

Auto-merge toggled on

yebblies added a commit that referenced this pull request Feb 14, 2014
Issue 7555 - ddoc whitespace issues due to version tags
@yebblies yebblies merged commit e1b18fd into dlang:master Feb 14, 2014
@9rnsr 9rnsr deleted the fix7555 branch February 14, 2014 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants