Bug 10862 – Assignment inside if condition still sometimes accepted

Consider: void main() { int a, b; if (a = b) {} } ./test.d(4): Error: assignment cannot be used as a condition, perhaps == was meant? Nice! Now consider: void main() { int a, b; if ((a = b) = 0) {} } This compiles diagnostic-free. The shape if (expr1 = expr2) should be disallowed at a grammatical level, i.e. during parsing.

Are you saying that if((a = b) == 0) should be disallowed or just if((a = b) = 0) The first syntax is what is used to shut up gcc's warnings about using the assignment operator in a condition, whereas the second clearly is using the assignment operator in the same way that if(a = b) is, and that's clearly illegal.

Whoever fixes this: Just be careful not to disallow: if (auto a = b) {}

(In reply to comment #1) > Are you saying that > > if((a = b) == 0) > > should be disallowed or just > > if((a = b) = 0) Only the latter. It fits the pattern "if (expr1 = expr2)".

(In reply to comment #3) > (In reply to comment #1) > > Are you saying that > > > > if((a = b) == 0) > > > > should be disallowed or just > > > > if((a = b) = 0) > > Only the latter. It fits the pattern "if (expr1 = expr2)". (...and there should be a space after "if". In fact, the compiler should enforce it :o).)

> (...and there should be a space after "if". In fact, the compiler should > enforce it :o).) LOL. Yeah, well, we're in trouble if the compiler is enforcing formatting. And to me, putting a space after the if is just wasted space, much as I know you like it.

(In reply to comment #5) > > (...and there should be a space after "if". In fact, the compiler should > > enforce it :o).) > > LOL. Yeah, well, we're in trouble if the compiler is enforcing formatting. And > to me, putting a space after the if is just wasted space, much as I know you > like it. Rightit'snotlikespacescontributetoreadabilityoranything.

> Rightit'snotlikespacescontributetoreadabilityoranything. There's a huge difference between not inserting a space between elements which are already separated by other grammar elements and not putting spaces between words, and even if you prefer having the space after the if for whatever reason, I don't see how you could think that if(expr) over if (expr) has an impact on legibility like not putting spaces between words would.

(In reply to comment #7) > > Rightit'snotlikespacescontributetoreadabilityoranything. > > There's a huge difference between not inserting a space between elements which > are already separated by other grammar elements and not putting spaces between > words. W.r.t. "if(" vs. "if (", it's not a big deal. However I have to argue about your statement. Someone was talking about the Fox toolkit recently, how it might be a good idea to port it to D. But then I had a look at it's codebase. Here's a glimpse of the code: ----- if(XGetSelectionOwner((Display*)getApp()->getDisplay(),getApp()->xdndSelection)!=xid){ fxwarning("%s::beginDrag: failed to acquire DND selection.\n",getClassName()); return false; } resizeElms(getApp()->xdndTypeList,numtypes); memcpy(getApp()->xdndTypeList,types,sizeof(FXDragType)*numtypes); getApp()->xdndNumTypes=numtypes; XChangeProperty((Display*)getApp()->getDisplay(),xid,getApp()->xdndTypes,XA_ATOM,32,PropModeReplace,(unsigned char*)getApp()->xdndTypeList,getApp()->xdndNumTypes); XChangeProperty((Display*)getApp()->getDisplay(),xid,getApp()->xdndActions,XA_ATOM,32,PropModeReplace,(unsigned char*)getApp()->xdndActionList,6); ----- Some of those lines span over 160 characters. I can barely see which argument becomes which parameter in these function calls, and everything is mushed together. There's a maximum effort of saving whitespace, as if having all characters on the screen is somehow going to enable someone to automatically process more information. We're not machines that parse grammars. Grammar rules or not, we need some breaks and some whitespace to be able to read and comprehend.

(In reply to comment #8) > if(XGetSelectionOwner((Display*)getApp()->getDisplay(),getApp()->xdndSelection)!=xid){ A: if (XGetSelectionOwner((Display*)getApp()->getDisplay(),getApp()->xdndSelection)!=xid){ B: if (XGetSelectionOwner ((Display*)getApp ()->getDisplay (),getApp ()->xdndSelection)!=xid){ C: if( XGetSelectionOwner( (Display*)getApp()->getDisplay(), getApp()->xdndSelection ) != xid ){ I'll take C, please!

@Andrej Mitrovic Well, while I completely agree that that code is horrible and hard to read, even that's not as bad as running words together, since it least then it's _possible_ to know for sure what the code says. And even if you consider not putting a space after an if to be akin to running words together, it's at most like running two of them together, not a whole sentence, so the impact is very different. But regardless, I don't really want to get into an argument about this. Code formatting style is a very subjective thing, and one person's perfect style could be another person's worst nightmare. If you prefer an extra space after the if, because you think that it improves legibility or for any other reason, that's fine. Format your code however you like. But personally, I don't think that adding the space helps legibility at all or provide any other benefit, and it takes up additional space, so I never do it in my code.

(In reply to comment #10) > it takes up additional space, so I never do it in my code. I'm baffled by this, you say it wastes spaces, yet look at how much space you're wasting in std.datetime: void _assertPred(string op, L, R) (L lhs, R rhs, lazy string msg = null, string file = __FILE__, size_t line = __LINE__) if((op == "<" || op == "<=" || op == "==" || op == "!=" || op == ">=" || op == ">") && You've saved one character and wasted 5 lines for a predicate, in a module that reaches 34000 lines.

(In reply to comment #11) > (In reply to comment #10) > > it takes up additional space, so I never do it in my code. > > I'm baffled by this, you say it wastes spaces, yet look at how much space > you're wasting in std.datetime: > > void _assertPred(string op, L, R) > (L lhs, R rhs, lazy string msg = null, string file = __FILE__, > size_t line = __LINE__) > if((op == "<" || > op == "<=" || > op == "==" || > op == "!=" || > op == ">=" || > op == ">") && > > You've saved one character and wasted 5 lines for a predicate, in a module that > reaches 34000 lines. Vertical alignment FTW! Besides... he saved on *6* characters! if ((op == "<" || op == "<=" || op == "==" || op == "!=" || op == ">=" || op == ">") && I don't care much about spaces before or after operators (except for people who put a space between a function name and its empty paren, eg: "doit ()" :puke:). That said, I'm a real bitch for vertical alignment, as well as clear repeatable instructions per line. There's something about the result that just makes the code flow when looking at it.

(In reply to comment #12) > That said, I'm a real bitch for vertical alignment Sure I like that too. (In reply to comment #12) > as well as clear repeatable instructions per line. That sounds like code duplication to me. The constraint could have been: if(op.canFind("<", "<=", "==", "!=", ">=", ">")) { } But then there are other code duplications which are just lazy, such as: if(msg.empty) { throw new AssertError(format(`_assertPred!"%s" failed: Return value of [%s] %s [%s] was [%s] instead of [%s].`, op, origLHSStr, op, rhs, result, expected), file, line); } else { throw new AssertError(format(`_assertPred!"%s" failed: Return value of [%s] %s [%s] was [%s] instead of [%s]: %s`, op, origLHSStr, op, rhs, result, expected, msg), file, line); } Why not reduce this to: string tail = msg.empty ? "." : format(": %s", msg); throw new AssertError(format(`_assertPred!"%s" failed: Return value of [%s] %s [%s] was [%s] instead of [%s]%s`, op, origLHSStr, op, rhs, result, expected, tail), file, line); Boom I saved you 8 lines. Don't tell me this is somehow slowing down performance, the cost of throwing an exception is much higher than allocating a small string. Plus AssertError is typically unrecoverable. All of this duplication adds up, and you end up with the massive std.datetime. Meanwhile someone believes they have to save that one whitespace character in the "if" statement. It's completely absurd.

Sure, it's just one space, but I also see no reason whatsoever to have it. It adds zero value IMHO. There are other places where I may think that extra spaces may be worth it, and you may not. I happen to value lining up arguments far more than saving space. I also typically don't mind using extra vertical space, whereas others really don't like it. And I'm not about to claim that the formatting in std.datetime is perfect or couldn't be improved, but whatever I did with it seemed reasonable to me at the time. Different people have different opinions on what improves or harms legibility and what is good and bad formatting. For the most part, it's very subjective. Personally, I happen to see no value in putting a space after the if, whereas you and Andrei like it. I don't think that you can objectively say that one is better than the other. Other people would be arguing that no space should go there but that spaces should be put inside the parens. It's all personal preference, and arguing about it doesn't get us anywhere.

no space after '(' or before ')' - evar

> no space after '(' or before ')' - evar On that, we agree, but it's a subjective thing. I've worked with folks who always do that and much prefer it.

(In reply to comment #16) > > no space after '(' or before ')' - evar > > On that, we agree, but it's a subjective thing. I've worked with folks who > always do that and much prefer it. Clearly. There is one argument though - in literary and mathematical use a whitespace can never follow a ')' or precede a ')'. The counter-argument is that program formatting rule does not need to necessarily follow literary or math rules, to which the counter-counter argument is it's better to not invent gratuitous departures, either.

Back to topic :] Fix was a bit easy but should work corretly. https://github.com/D-Programming-Language/dmd/pull/2491

Commits pushed to master at https://github.com/D-Programming-Language/dmd https://github.com/D-Programming-Language/dmd/commit/34e4acf514eff195b76c614102f8050255f79b36 fix issue 10862 - Assignment inside if condition still sometimes accepted https://github.com/D-Programming-Language/dmd/commit/297a3e0980a6938be52d2f1209fd29aecf96df95 Merge pull request #2491 from hpohl/10862 fix issue 10862 - Assignment inside if condition still sometimes accepted

Confirmed fixed.