This is the mail archive of the gcc-bugs@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug c++/19199] [3.3/3.4/4.0 Regression] Wrong warning about returning a reference to a temporary


------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-04 20:18 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold

On Apr  4, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> My apologies yet again for not being more explicit about all of the
> things that were wrong (and or I was unhappy with) with your proposed
> solution.  I'd hoped that I was clear in the comments in the bugzilla
> thread, and that you'd appreciate the issues it addressed.

I didn't.  In fact, I only saw your responses in bugzilla early last
week, since bugzilla e-mail I get goes to a folder that I don't read
very often because of all the traffic in gcc-prs.

> [1] The use of pedantic_lvalues to identify the non-C front-ends,
> adversely affects code generation in the Java, Fortran and Ada
> front-ends.

Indeed.  I'd misunderstood what pedantic_lvalues stood for, and
thought it could be used to distinguish C++ from other languages, not
C from other languages.  My mistake.  That's why it was never clear to
me that you disliked its use, and why.

> The use of COND_EXPRs as lvalues is unique to the
> C++ front-end

I don't know about that.  Among all the languages supported by GCC,
I'm only familiar with C, C++ and Java.

> [3] Your patch is too invasive.  Compared to the four-line counter
> proposal that disables just the problematic class of transformations,
> your much larger patch inherently contains a much larger risk.  For
> example, there is absolutely no need to modify the source code on the
> "A >= 0 ? A : -A  ->   abs (A)" paths as these transformations could
> never interfere with the lvalueness of an expression.

Granted.  OTOH, the patch faulted in trying to be overly consistent in
potentially-broken code, as opposed to keeping potentially-broken code
broken.

> Additionally, once one of the longer term solutions proposed by Mark
> or me is implemented, all of these workarounds will have to be undone/
> reverted.  By only affecting a single clause, we avoid the potential
> for leaving historic code bit-rotting in the tree.

If you're convinced that's the only clause that triggers the problem,
great.  I wasn't.

> [4] Despite your counter claims you approach *does* inhibit the ability
> of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR
> nodes.

Indeed.  I still had the behavior from an earlier version of the
patch, in which cond_expr returned different values from
maybe_lvalue_p depending on whether we were in gimple form or not.
Sorry about that.

> [5] For the immediate term, I don't think its worth worrying about
> converting non-lvalues into lvalues,

No worries here, all of my effort was toward preserving lvalueness.

> And although, not serious enough to warrant a [6], it should be pointed
> out that several of your recent patches have introduced regressions.

Oh, like the patch in which I did check in a testcase after
bootstrapping it on amd64-linux-gnu, although I didn't realize it
still failed on i686-pc-linux-gnu, a host on which I don't bootstrap
very often any more because my 32-bit hardware has become too slow?

> Indeed, you've not yet reported that your patch has been sucessfully
> bootstraped or regression tested on any target triple.

Did I have to, before checking the patch in?  I didn't think so.

> Indeed, your
> approach of posting a patch before completing the prerequisite testing
> sprecified/stressed in contribute.html, on more than one occassion
> recently resulted in the patch having to be rewritten/tweaked.

And the need for rewriting/tweaking is exactly the reason why I choose
to post patches early.  Quite often, patches don't go in as they're
posted, since maintainers often request additional minor tweaks.  At
about 24 hours per test cycle, and about as much for patch review, I
don't think posting a patch proposal early would hurt anyone.  In
general, I'm asking for feedback on the approach, rather than on the
exact patch spelling.  If people find the patch to be good as is,
great.  Otherwise, sometimes I can still tweak the patch and get it
into the next build&test cycle.

And, more importantly, I like to describe the problem and the solution
while it's still fresh in my mind.  If I wait 1 or 2 days to post it,
I won't be as precise in my description, and if questions arise, I may
fail to answer correctly.

> Indeed, as witnessed in "comment #17", I've already approved an
> earlier version your patch once, only to later discover you were
> wasting my time.

If you find that reviewing patch proposals is wasting your time, I'm
sorry.  In general, I run a `make all; make check-gcc or make
check-g++' on a previously-built tree before starting a full bootstrap
and posting the patch, but this is not enough to catch all errors.  In
fact, a full bootstrap and test cycle isn't enough either, since we
don't all use the same hosts.  So, patches are going to be posted that
need revision.  Since most of my patches have minor revisions
requested, not posting the patch for early feedback wastes a lot of my
time.  How do we find a balance?

> As a middle-end maintainer, having been pinged by the release manager
> that we/you weren't making sufficient progress towards addressing the
> issues with your patch,

Only days after that did I find you'd added notes to bugzilla, while
an e-mail reply would have caught my attention immediately.

> Perhaps, I should ask the counter question to your comment #21?
> In what way do you feel that the committed patch isn't clearly
> superior to your proposed solution?  p.s. Thanks for spotting my
> mistake of leaving a bogus comment above maybe_lvalue_p.

I can't immediately convince myself that it covers all the
potentially-faulty cases, and it tests for lvalueness after stripping
NOP_EXPRs, which may prevent optimization in case the operand of the
NOP_EXPR looks like an lvalue, but the NOP_EXPR itself is clearly an
rvalue.

It's easy enough to get the lvalue variable to take in_gimple_form
into account, and we can move the test for the language name to the
assignment to lvalue instead of using pedantic_lvalues, if you really
think that's better than a langhook.  Personally, I'd go with a
lang-specific boolean that defaults to false, to tell whether a
cond_expr/min_expr/max_expr can be an lvalue.  I thought
pedantic_lvalues was just that, but it's now obvious that I was
mistaken.



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]