Hello! On Thu, Jan 10, 2008 at 11:54:46PM +0100, Richard B. Kreckel wrote:
Ouch, that patch is ugly as sin.
Unfortunately, this ugliness is imposed by the standard (ANSI C++).
But if, as a consequence, it will help someone provide DLLs of CLN, well, then maybe it should be applied.
The patch gets rid of some standard violating code. As a consequence, it helps me to provide DLLs [1].
However, I'm worried about the maintainability: Can you, please, write a comment explaining what you're doing at some suitable place in the sources?
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?
(Oh, and don't worry about that cln_1-1 branch: I'll release CLN 1.2.0 really soon now.)
The patch was written specifically for CLN version 1.1, since I use that version of CLN (I'm not going to upgrade anytime soon). I've forward ported it to the development branch (that take me less than 5 minutes).
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 Other cases are more difficult to analyse. Anyway, the compiler is free to NOT inline functions, and the code[r] should cope with that. Best regards, Alexei [1] http://theor.jinr.ru/~varg/web/proj/ginac/woe32 -- All science is either physics or stamp collecting.