www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 10862] New: Assignment inside if condition still sometimes accepted

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862

           Summary: Assignment inside if condition still sometimes
                    accepted
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: andrei erdani.com


--- Comment #0 from Andrei Alexandrescu <andrei erdani.com> 2013-08-20 14:40:37
PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 20 2013
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


Jonathan M Davis <jmdavisProg gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg gmx.com


--- Comment #1 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-20 14:52:21
PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrej.mitrovich gmail.com


--- Comment #2 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-08-20
14:54:29 PDT ---
Whoever fixes this: Just be careful not to disallow:

if (auto a = b) {}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #3 from Andrei Alexandrescu <andrei erdani.com> 2013-08-20 14:57:08
PDT ---
(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)". -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #4 from Andrei Alexandrescu <andrei erdani.com> 2013-08-20 14:58:03
PDT ---
(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).) -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #5 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-20 15:43:58
PDT ---
 (...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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #6 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-08-20
16:10:02 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #7 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-20 16:35:35
PDT ---
 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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #8 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-08-20
17:21:26 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


Nick Sabalausky <cbkbbejeap mailinator.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cbkbbejeap mailinator.com


--- Comment #9 from Nick Sabalausky <cbkbbejeap mailinator.com> 2013-08-20
19:38:39 PDT ---
(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! -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #10 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-20 21:51:59
PDT ---
 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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 20 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #11 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-08-21
03:48:27 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


monarchdodra gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |monarchdodra gmail.com


--- Comment #12 from monarchdodra gmail.com 2013-08-21 05:23:08 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #13 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-08-21
06:33:11 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #14 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-21 10:54:30
PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #15 from Andrei Alexandrescu <andrei erdani.com> 2013-08-21
13:56:41 PDT ---
no space after '(' or before ')' - evar

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #16 from Jonathan M Davis <jmdavisProg gmx.com> 2013-08-21 16:27:09
PDT ---
 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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #17 from Andrei Alexandrescu <andrei erdani.com> 2013-08-21
16:31:26 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


Henning Pohl <henning still-hidden.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic, pull
                 CC|                            |henning still-hidden.de


--- Comment #18 from Henning Pohl <henning still-hidden.de> 2013-08-21 19:10:07
PDT ---
Back to topic :]

Fix was a bit easy but should work corretly.

https://github.com/D-Programming-Language/dmd/pull/2491

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 21 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862



--- Comment #19 from github-bugzilla puremagic.com 2013-08-22 00:07:17 PDT ---
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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 22 2013
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=10862


monarchdodra gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


--- Comment #20 from monarchdodra gmail.com 2013-08-22 01:38:02 PDT ---
Confirmed fixed.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 22 2013