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

["roger at eyesopen dot com" <gcc-bugzilla at gcc dot gnu dot org>]

------- Additional Comments From roger at eyesopen dot com  2005-04-04 16:02 -------
Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold


Hi Alex,

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.

Problems with your approach:

[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.  The use of COND_EXPRs as lvalues is unique to the
C++ front-end, so ideally a fix shouldn't regress code quality on
innocent front-ends.  Certainly, not without benchmarking to
indicate how significant a performance hit, these other languages
are taking prior to a release.

[2] The pedantic_lvalues flag is itself a hack used by the C
front-end, that is currently being removed by Joseph in his clean-up
of the C parser.  Adding this use would block his efforts, until an
alternate solution is found.  Admittedly, this isn't an issue for
the 4.0 release, but creates more work or a regression once this is
removed from mainline.

[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.

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.

[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.  Once the in_gimple_form flag is set, fold-const.c is able to
optimize "A == B ? A : B  ->  B" even when compiling C++, as it knows
that a COND_EXPR can't be used as an lvalue in the late middle-end.

[5] For the immediate term, I don't think its worth worrying about
converting non-lvalues into lvalues, the wrong-code bugs and diagnostic
issues are related solely to the lvalue -> non-lvalue transition.
At this stage of pre-release, lower risk changes are cleary preferrable,
and making this change will break code that may have erroneously compiled
in the past.  Probably, OK for 4.0, but not for 3.4 (which also exhibits
this problem).


And although, not serious enough to warrant a [6], it should be pointed
out that several of your recent patches have introduced regressions.
Indeed, you've not yet reported that your patch has been sucessfully
bootstraped or regression tested on any target triple.  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.  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.


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, I took the opportunity to apply a fix that
was within my authority to commit.  If you'd had made and tested the
changes that I requested in a timely manner, I'm sure I'd have approved
those efforts by now.

My apologies again for not being blunt earlier.  My intention was
that by listing all of the benefits of an alternate approach in
comment #19 of the bugzilla PR, I wouldn't have to explicitly list
them as the defficiencies of your approach.  Some people prefer the
carrot to the stick with patch reviews [others like RTH's "No"].


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.

Roger
--



-- 


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