Dear Alexei, Alexei Sheplyakov wrote:
Ouch, that patch is ugly as sin.
Unfortunately, this ugliness is imposed by the standard (ANSI C++).
Yes, I'm aware of that.
Non-diff part of my previous email is supposed to explain the patch. I'm afraid more details will only obfuscate the source. Anyway, here it is:
CLN wants to inline some functions in some places and not in others. The reasons to do so are explained here: http://www.ginac.de/pipermail/ginac-list/2003-November/000433.html
However, MAYBE_INLINE thing is a wrong way. First of all, the `inline' keyword is only a recommendation, not an order. The compiler is free to not inline a function (IMHO its decisions are correct more often than not). And this is what actually happens, see, e.g.
http://www.ginac.de/pipermail/cln-list/2007-October/000317.html
As a side note, the compiler is free to inline functions even if it is NOT marked as inline (this can be disabled with __attribute__((noinline)) in GNU C/C++).
Secondly, in ANSI C++ a function *must* be either inline in *every* translation unit, or non-inline in *every* translation unit. Hence, MAYBE_INLINE is not valid C++ (although it's valid C). The compiler does not check for violations of this rule (as per standard), so MAYBE_INLINE kind of works on some platforms (and some versions of the GNU compiler and some misterious compiler/linker flags. This is not speculation, I used to get bizzare link errors even on Linux with g++ 3.3 and CXXFLAGS="-O2 -mcpu=ev6").
The patch renames internal versions of "simple" functions and makes them (static) inline. Thus, the code does not violate the standard, and link errors don't happen. However, everything comes at a price. It's necessary to replace almost every invocation of a function `foo' with `inline_foo'. That `almost' is the worse thing: in some places `foo' should NOT be renamed to `inline_foo'. This is ugly, but I don't see any better *standard-compliant* way to achive the goal.
Does this explantion sound any better? Is it OK to include it in the code?
Yes, thanks a lot. But I'm still having a hard time understanding how the combination of attributes 'flatten' and 'always_inline' work out. Please put in a little comment about that, directly at the point of definition in the source code. It would be quite helpful, I think. Can you, please, do this and also incorporate Bruno's suggestion, then resend the entire patch? I'm waiting with the release until this is checked in. Thanks a lot!
And one last question: Do you have any idea why it's enough converting only these few occurrences?
In all of these cases inlining the function in question is very difficult or even impossible.
The most obvious is zerop. In cl_I_ring.cc
112 static cl_number_ring_ops<cl_I> I_ops = { 113 cl_I_p, 114 equal, 115 zerop, 116 operator+, 117 operator-, 118 operator-, 119 operator*, 120 square, 121 expt_pos 122 };
Here, the address of zerop is taken, hence, the compiler is forced to emit out-of-line copy. The same applies to cl_RA_ring.cc
Aha, thank you. Cheers -richy. -- Richard B. Kreckel <http://www.ginac.de/~kreckel/>