From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32562 invoked by alias); 4 Apr 2005 16:02:55 -0000 Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Archive: List-Post: List-Help: Sender: gcc-bugs-owner@gcc.gnu.org Received: (qmail 32452 invoked by alias); 4 Apr 2005 16:02:46 -0000 Date: Mon, 04 Apr 2005 16:02:00 -0000 Message-ID: <20050404160246.32450.qmail@sourceware.org> From: "roger at eyesopen dot com" To: gcc-bugs@gcc.gnu.org In-Reply-To: <20041230105911.19199.lars@trolltech.com> References: <20041230105911.19199.lars@trolltech.com> Reply-To: gcc-bugzilla@gcc.gnu.org Subject: [Bug c++/19199] [3.3/3.4/4.0/4.1 Regression] Wrong warning about returning a reference to a temporary X-Bugzilla-Reason: CC X-SW-Source: 2005-04/txt/msg00322.txt.bz2 List-Id: ------- 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