This is the mail archive of the gcc-patches@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]

Re: [RFC][4.4] Conversion of tree.h STRIP_* macros into static inline functions


Kaveh R. GHAZI wrote:
On Tue, 18 Dec 2007, Simon Baldwin wrote:

Attached is a revised version of this patch, incorporating review
comments; thanks for the feedback.

+ static inline tree __attribute__((warn_unused_result))
+ STRIP_NOPS (const_tree exp)

You can't use a naked __attribute__ keyword like that because that will break bootrapping with non-gcc compilers. It will also break bootstrapping with gcc versions released prior to adding the __warn_unused_result__ feature.

I suggest adding a macro ATTRIBUTE_WARN_UNUSED_RESULT in
include/ansidecl.h like so: ...

Thanks for the suggestion.


I've added the new macro to include/ansidecl.h, and changed the new function definitions to use it.



Andrew Pinski wrote:
On 12/19/07, Kaveh R. GHAZI <ghazi@caip.rutgers.edu> wrote:
On Tue, 18 Dec 2007, Simon Baldwin wrote:

Attached is a revised version of this patch, incorporating review
comments; thanks for the feedback.

+ static inline tree __attribute__((warn_unused_result))
+ STRIP_NOPS (const_tree exp)
You can't use a naked __attribute__ keyword like that because that will
break bootrapping with non-gcc compilers. It will also break
bootstrapping with gcc versions released prior to adding the
__warn_unused_result__ feature.

Really I think any use of warn_unused_result is wrong and should not
be included. You are forcing a warning that can be avoided if the
person knows what he/she doing.

Isn't that the definition of a warning; a note about something that's avoidable if you know to avoid it?


Tom's suggestion to add warn_unused_result here was not so much a concern not for writers of new code, but for code merges, where the subtle change in functionality would be easily missed. It's a valid thing to worry about. Adding the warning, therefore, seems useful.

Really you should not be changing the behavior of
STRIP_NOPS/STRIP_SIGN_NOPS, etc. There is a simple way around to
changing their behavior, still have a macro but pass address to the
new inline function and act on that. Or even have them take an
address to a tree. And then you can also get rid of the crappy
CONST_CAST_TREE.

These macros were representative of coding practices from a decade or two ago, and deserved to be brought up to date.


Functions offer an element of type safety, are easier to handle in a debugger, and can be used in more conventional and comfortable contexts than can a macro that modifies a variable value. And there is no apparent performance penalty.

The cast is necessary because STRIP_* are called with both tree and const_tree values. Making them take a (non-const) tree means that a cast would then be necessary for each caller that passes a const_tree argument. Making them take a const_tree, on the other hand, means that you have, if you want to avoid casting, to return a const_tree; this makes them awkward for callers that pass a (non-const) tree argument.

Also I noticed you have some coding style violations:
+ return CONST_CAST_TREE(exp);
there is missing a space there.

Thanks; fixed.


I did a quick scan of gcc source -- it turns out there are more uses of CONST_CAST_TREE (excluding mine) present with this violation, than without.

And this just looks like some child started to draw all over the wall
(that reminds me, I need to deal with a couple of children soon):
! cond = STRIP_NOPS (fold_build2 (LT_EXPR, boolean_type_node, t1, t2));

This is a silly objection. The revised version of this code is not worse than its original.



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