[CLN-list] Re: MAYBE_INLINE patch

[Richard B. Kreckel]

Dear Alexei,

Alexei Sheplyakov wrote:
> On Sat, Jan 12, 2008 at 12:59:27AM +0100, Richard B. Kreckel wrote:
>> But I'm still having a hard time understanding how the combination of
>> attributes 'flatten' and 'always_inline' work out.
> 
> These attributes make the compiler inline functions more aggressively,
> so no out of line copies (which cause link failure) produced.

*That* should be explained in source code comments.

>> Can you, please, do this and also incorporate Bruno's suggestion, then 
>> resend the entire patch?
> 
> I found out my patch was incomplete.

Which makes me wonder: what's the definition of 'complete', here? Does 
it work or not? Does it depend on compiler flags like -finline-limit?

>                                     In some places non-inline versions of
> functions was used intstead of inline ones. In principle, they never got
> inlined anyway (this was exactly the reason of linker errors, and that's why
> I made the patch in first place). I've tried to replace MAYBE_INLINE
> the Right Way (TM), however, it turned out to be somewhat difficult. I.e.
> it's possible to replace either all of MAYBE_INLINEs or none of them. Thus,
> the correct patch would be even more ugly. Also I failed to convert a couple
> of (most) obscure instances:
> 
> 1. src/real/misc/cl_R_eqhashcode.cc
> 
> 20 #include "cl_I_eqhashcode.cc"
> 21 #include "cl_SF_eqhashcode.cc"
> 22 #include "cl_FF_eqhashcode.cc"
> 23 #include "cl_DF_eqhashcode.cc"
> 24 #include "cl_LF_eqhashcode.cc"
> 
> Why equal_hashcode(const cl_RA&) is not inlined, but other type-specific equal_hashcode
> functions are?
> 
> 2. src/rational/misc/cl_RA_eqhashcode.cc
> 
> Why equal_hashcode(const cl_I&) is not inlined?
> 
> 3. include/cln/float.h
> 
> 23 struct cl_idecoded_float { 
> 24         cl_I mantissa;
> 25         cl_I exponent;  
> 26         cl_I sign;
> 27 // Constructor. 
> 28         cl_idecoded_float () {}
> 29         cl_idecoded_float (const cl_I& m, const cl_I& e, const cl_I& s) : mantissa(m), exponent(e), sign(s) {}
> 30 };
> 
> Why sign is cl_I? It looks like bool would be enough.

Why not cl_I? Remember that small integers are immediate.

> 4. src/base/string/cl_st_concat2.cc
> 
> Why it is necessary to inline cl_make_heap_string? Is it really performance
> critical? And why CLN needs its own type for strings, what's wrong with
> std::string?

I've also been pondering the removal of that own string class and 
replacing it with std::string. But it has never been a priority and the 
change would be somewhat intrusive. That will have to wait until some 
time after 1.2.0 now.

As to why it is necessary to inline cl_make_heap_string? I can't imagine 
it is critical.

   -richy.
-- 
Richard B. Kreckel
<http://www.ginac.de/~kreckel/>