www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 4231] New: Solidary opUnary Postincrement and Postdecrement user defined operators are broken.

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

           Summary: Solidary opUnary Postincrement and Postdecrement user
                    defined operators are broken.
           Product: D
           Version: unspecified
          Platform: Other
        OS/Version: All
            Status: NEW
          Keywords: rejects-valid
          Severity: regression
          Priority: P3
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: sandford jhu.edu


--- Comment #0 from Rob Jacques <sandford jhu.edu> 2010-05-24 21:45:40 PDT ---
It appears that the operator re-writing for the opUnary Post increment and Post
decrement operators is incorrect for single line expressions.

struct Foo{
    int opUnary(string op)() { return 1; }
}

void main() {
    Foo foo;
    foo++;   // Error: var has no effect in expression (__tmp608)

}

I've marked this as a regression since this was possible (and still is) using
the old operator overloading style.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 24 2010
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Don <clugdbug yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |performance
                 CC|                            |clugdbug yahoo.com.au
            Summary|Solidary opUnary            |Solitary opUnary
                   |Postincrement and           |Postincrement and
                   |Postdecrement user defined  |Postdecrement user defined
                   |operators are broken.       |operators are broken.


--- Comment #1 from Don <clugdbug yahoo.com.au> 2010-06-01 12:26:55 PDT ---
Root cause: the temporary variable should not be created if the return value
isn't required. This would mean that when the value is not required,
preincrement and postincrement are identical, resulting in optimal performance.

Note that the same situation (no return value required) occurs inside comma
expressions.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 01 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Brad Roberts <braddr puremagic.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |braddr puremagic.com


--- Comment #2 from Brad Roberts <braddr puremagic.com> 2010-06-01 17:20:53 PDT
---
Doesn't the optimizer take care of eliminating the unused temporary and copy?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 01 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Andrei Alexandrescu <andrei metalanguage.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrei metalanguage.com


--- Comment #3 from Andrei Alexandrescu <andrei metalanguage.com> 2010-06-01
17:29:18 PDT ---
Thanks, Don!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 01 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #4 from Don <clugdbug yahoo.com.au> 2010-06-02 08:53:44 PDT ---
(In reply to comment #2)
 Doesn't the optimizer take care of eliminating the unused temporary and copy?
Good question. Maybe it does. It sees: auto t = e, foo(e), t; Does it know in general that foo() cannot reach t? BTW -- should the compiler be allowed to eliminate the temporary, if there's a postblit? Ie, is it *forced* to perform the rewrite: (auto t = e, --e, t) even if the return value is not used? The optimizer certainly couldn't eliminate it in the general case, but it'd be possible if the front-end is allowed to elide it. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jun 02 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #5 from Andrei Alexandrescu <andrei metalanguage.com> 2010-06-02
08:57:50 PDT ---
That looks like a very specialized optimization to me. In particular, if the
postblit has side effects, the optimizer must have advanced knowledge in order
to elide it. This is a path that C++ has taken with copy constructor elision,
and it's not a path we should take.

I think the language definition should clarify that postincrement and
postdecrement are lowered into their pre- counterparts if the result is not
taken.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 02 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #6 from Brad Roberts <braddr puremagic.com> 2010-06-02 10:22:56 PDT
---
I was thinking about the code post-inlining.  MOST of the time the operators
will be inlined and at that point it should be dead simple for it to eliminate
dead stores and thus the temporaries would just go away, no special knowledge
or techniques required.  Without inlining, yeah, it can't make assumptions what
can occur inside the function calls.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 02 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #7 from Andrei Alexandrescu <andrei metalanguage.com> 2010-06-02
10:53:53 PDT ---
(In reply to comment #6)
 I was thinking about the code post-inlining.  MOST of the time the operators
 will be inlined and at that point it should be dead simple for it to eliminate
 dead stores and thus the temporaries would just go away, no special knowledge
 or techniques required.  Without inlining, yeah, it can't make assumptions what
 can occur inside the function calls.
Inlining is irrelevant. If a this(this) has a writeln() in it, the optimizer must honor it no questions asked. That's why elision must come from a higher level. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jun 02 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #8 from Brad Roberts <braddr puremagic.com> 2010-06-02 11:19:00 PDT
---
(In reply to comment #7)
 Inlining is irrelevant. If a this(this) has a writeln() in it, the optimizer
 must honor it no questions asked. That's why elision must come from a higher
 level.
It's entirely relevant for the original issue: removal of unnecessary temporaries. Yes, there are opportunities for the language to define away some parts, but that's a separate discussion for a separate bug report. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jun 02 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #9 from Don <clugdbug yahoo.com.au> 2010-06-06 13:03:45 PDT ---
Bug 3966 is the same as this one. But I'm loathe to mark either as duplicate
since 4231 contains useful discussions and 3966 has a vote.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 06 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Don <clugdbug yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch


--- Comment #10 from Don <clugdbug yahoo.com.au> 2010-06-08 00:01:20 PDT ---
This patch only stops the side effect message, it doesn't turn x++; into ++x;
Note that this patch deals with more difficult cases such as:

struct Foo{
    int opUnary(string op)() { return 1; }
}

void main() {
    Foo foo;
    int w;
    ++w, foo++;
}
----

Index: expression.c
===================================================================
--- expression.c    (revision 526)
+++ expression.c    (working copy)
   -8520,6 +8520,14   

 int CommaExp::checkSideEffect(int flag)
 {
+    // Check for compiler-generated code of the form  auto __tmp, e, __tmp;
+    // In such cases, only check e for side effect (it's OK for __tmp to have
no side effect).
+    CommaExp * firstComma = this;
+    while (firstComma->e1->op == TOKcomma)
+        firstComma = (CommaExp *)firstComma->e1;
+    if (firstComma->e1->op == TOKdeclaration && e2->op == TOKvar)
+        return e1->checkSideEffect(flag);
+
     if (flag == 2)
         return e1->checkSideEffect(2) || e2->checkSideEffect(2);
     else

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #11 from Brad Roberts <braddr puremagic.com> 2010-06-08 00:24:37
PDT ---
Hrm.. I haven't studied the side effect code enough.  Do you know why
Comma:Exp::checkSideEffect isn't just:

return e1->checkSideEffect(flag) || e2->checkSideEffect(flag)

ie, no conditional.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #12 from Don <clugdbug yahoo.com.au> 2010-06-08 01:22:02 PDT ---
(In reply to comment #11)
 Hrm.. I haven't studied the side effect code enough.  Do you know why
 Comma:Exp::checkSideEffect isn't just:
 
 return e1->checkSideEffect(flag) || e2->checkSideEffect(flag)
 
 ie, no conditional.
If flag isn't 2, you still want to check for a useless subexpression. Eg. int x; int y; ++y, x; This should still be an error, since x; has no effect. This shows me that my patch isn't quite right, it will erroneously allow void main() { Foo foo; int w; foo++, w; } -- Revised patch (added one line): should ensure that the created variable is the same as the one which is returned. CommaExp * firstComma = this; while (firstComma->e1->op == TOKcomma) firstComma = (CommaExp *)firstComma->e1; if (firstComma->e1->op == TOKdeclaration && e2->op == TOKvar + && ((DeclarationExp *)firstComma->e1)->declaration == ((VarExp*)e2)->var) return e1->checkSideEffect(flag); -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jun 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #13 from Brad Roberts <braddr puremagic.com> 2010-06-08 02:07:14
PDT ---
Something about that code bugs me, but I'm having trouble deciding exactly what
it is.

Part of it is that there's redundant work.  Move the new code inside the else
block?

Part of it is also that it presumes a good bit about the structure of the tree
inside a comma expression.  The comment suggests that it can only come from
generated code w/in the compiler.  How true is that?  How future proof is it?

Anyway, maybe my subconscious will figure out what's really bugging me while I
sleep.  More tomorrow... if anything comes to me.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231



--- Comment #14 from Don <clugdbug yahoo.com.au> 2010-06-08 04:13:39 PDT ---
(In reply to comment #13)
 Something about that code bugs me, but I'm having trouble deciding exactly what
 it is.
 
 Part of it is that there's redundant work.  Move the new code inside the else
 block?
Although the code in the else block is the same, it's for a very different reason. But I'm not sure it's correct. For example, int x; x, ++x; doesn't raise an error. Yet the first x has no effect! Shouldn't the part in the else clause be e1->sideeffect() && e2->sideeffect() ?
 Part of it is also that it presumes a good bit about the structure of the tree
 inside a comma expression.  The comment suggests that it can only come from
 generated code w/in the compiler.  How true is that?  How future proof is it?
Declarations are not legal inside comma expressions. But the compiler generates them in several places. They are also used in implementing struct constructors and postblit, for example. There's definitely something a bit weird about the compiler generating code that couldn't get past the parsing stage. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Jun 08 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Don <clugdbug yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs eml.cc


--- Comment #15 from Don <clugdbug yahoo.com.au> 2010-06-09 07:57:14 PDT ---
*** Issue 3966 has been marked as a duplicate of this issue. ***

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 09 2010
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4231


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |bugzilla digitalmars.com
         Resolution|                            |FIXED


--- Comment #16 from Walter Bright <bugzilla digitalmars.com> 2010-06-09
17:08:15 PDT ---
http://www.dsource.org/projects/dmd/changeset/530

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jun 09 2010