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



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



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



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




PDT ---


 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




PDT ---




 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




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




16:10:02 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.


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




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




17:21:26 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.


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



19:38:39 PDT ---


 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




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




03:48:27 PDT ---


 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







 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




06:33:11 PDT ---


 That said, I'm a real bitch for vertical alignment


Sure I like that too.



 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




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




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




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




16:31:26 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.


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



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




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


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



Confirmed fixed.

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