Fix Issue 2659 - Deprecate using the return of a comma expression#5737
Fix Issue 2659 - Deprecate using the return of a comma expression#5737yebblies merged 1 commit intodlang:masterfrom mathias-lang-sociomantic:coma-operator-must-die
Conversation
|
|
So, e.g. this code would trigger a deprecation warning?
That would make it rather hard to convert C-Code to D.. |
|
@Dgame : That's multiple declaration, and it's fine. Look at L28 on the changelog where it's used. |
|
I think we can disallow |
That's reasonable. However it looks like a bigger special case to me than the proposed solution. In addition, |
|
@yebblies you mean disallow usage of the comma expression result, or specifically assignment? Because I think this is also bad: void foo(int x) {}
void main()
{
int a, b;
foo((++a, b));
} |
|
And of course the example from TDPL and Ali's talk: synchronized(locka, lockb) // only locks lockb
{
} |
|
Disallow using the result. |
|
@Geod24 We can go in stages. First may be as Daniel specified, then deprecate such statements and suggest the obvious fix. Then remove comma expressions altogether (except for loop increments of course ;) If we eventually end up using comma for expression tuples, then we can move the for increment clause into being a tuple. |
That's a possibility. Before going down any path, we should probably assess how needed it is. Separating deprecation in different stages only makes sense if it significantly reduce the cost of transitioning for user code. If the occurrences are low enough, splitting it in multiple release might induce higher costs. Should we take this to the NG ? |
|
It may be a case of creeping acceptance. Whereas saying "we are going to remove all uses of comma operators" may be taboo, saying "we'll remove only the really bad uses of comma operators". Then if we don't get to the holy grail of eliminating comma operators (or reinterpreting them as equivalent feature such as tuples), at least we have some improvement that fixes a ton of bugs. But eliminating the bad uses may reveal that we can later easily move people on board the no comma train. But let's discuss as whole community. |
|
The idea is that if So code that relies on the type of a comma expression would break, but code that just relies on both arguments being evaluated would not. It has the additional (and somewhat more urgent) benefit of eliminating most comma expression bugs. |
|
@schuetzm : Thanks for the links and the previous attempts, third time's a charm!
And that is the motivation behind this P.R. I'm not proposing anything related to built-in tuple here. I propose that we deprecate a construct that is known to cause bugs, and that provides little to no advantage over the other, much more common construct(s), except in one single case which is not deprecated. If we make this enhancement depends on the future of the comma operator for tuples, we can already close this P.R. because it's not going to be merged. Regarding code breakage, again we should assess how much code is impacted. I didn't find any occurrence in Phobos (because they use Dscanner, thanks @Hackerpilot ), and only 3 in Druntime, one of which was a bug. |
#3943 would detect all comma bugs without breaking code like: if (foo)
x++, y++;Despite there being a trivial fix, it seems better not to risk breaking anyone's code, particularly when this pull probably doesn't simplify the compiler/language more than #3943. I agree ideally we would merge this pull, but for stability it's better to be conservative and never break people's code without good reason. |
|
Furthermore, there's the argument about making C/C++ code harder to port for no reason. |
I agree we should not make hasty decisions, or taking unreasonable path when changing the language. But this is not an on the spot decision, it's been discussed for at least 7 years. So far the thread on the forum only consist of yea, and I've yet to see a code construct that cannot be trivially rewritten and/or doesn't look better. |
|
The solution I've outlined before has the enormous advantage of being pre-approved. |
src/parse.d
Outdated
| } | ||
|
|
||
| Expression parseExpression() | ||
| Expression parseExpression(bool allow_commaexp = false) |
There was a problem hiding this comment.
is this parameter useful if it just prints a deprecation warning and wasn't used before?
|
@yebblies : I gave a shot at implementing a deprecation on using the return type of the |
|
I'm not sure it's possible without changing semantic a little. My suggestion would be to have a flag in |
Both cases still work, because the second case assign 'a' to 'x', not 'b', so this should work if we don't want to break 'legit' code. Anyway, updated (sorry for the delay). |
|
Only occurrence left: dlang/phobos#4348 |
Something like that. |
|
Fixed, now green. |
|
Please move the removal of comma exps in dmd's source and test suite into another PR. |
Geod24
changed the title
|
Renamed the P.R. + rebased on master |
|
Ping @yebblies @9rnsr @WalterBright : How does that look to you ? |
changelog.dd
Outdated
| module comma; | ||
|
|
||
| void main () { | ||
| size_t aggr; |
There was a problem hiding this comment.
Nitpick - indentation seems a bit off.
|
Updated according to @ZombineDev 's comments. |
src/statement.d
Outdated
| statements.insert(i, flt); | ||
| continue; | ||
| } | ||
| // Allow CommaExp in ExpStatement because return isn't used |
There was a problem hiding this comment.
Why isn't this part of ExpStatement.semantic?
There was a problem hiding this comment.
It may be worth encapsulating this check-and-set pattern into a function, eg discardCommaResult that walks the expression tree if necessary.
|
Updated |
| { | ||
| /// This is needed because AssignExp rewrites CommaExp, hence it needs | ||
| /// to trigger the deprecation. | ||
| const bool isGenerated; |
There was a problem hiding this comment.
This isn't actually private in D. I'd suggest just leaving it public, we're not systematically using member protection yet in D.
There was a problem hiding this comment.
One has to start :)
Removed it from the C++ file.
This should weed out most of the bugs.
|
Auto-merge toggled on |
|
Yay ! Thanks a lot :) |
|
This is awesome!! |
11 participants
The comma operator is a constant source of confusion / bugs.
In all use case, code using comma operator can be rewritten without hurting readability. The exception is the increment part of
forloops, where it is pretty handy.This P.R. introduce a deprecation message for every usage that isn't in a for loop increment.
Example of fixes needed outside of DMD:
Note:
There were some
CommaExpin the test that I had to silence using-d.The underlying reason is that deprecations message shouldn't require those tests to change (and the change was pretty big since I would have had to update all line numbers of error messages...).
Ultimately, in
forstatement, I think we should recommend using something along the line of{ a.popFront; b.popFront; }instead, but it shouldn't introduce any performance hit, which is not currently the case.Mandatory ping to @andralex / @WalterBright as this is a language change.