maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Toby Thain <toby@xxxxxxxxxxxxxxxxxxx> writes:

>> Just out of curiosity, what is the reason for this requirement?
>
> Path assumptions in the autotools.
>
> $ head `which aclocal`
> #!/usr/local/bin/perl -w

Ah, I see, thanks for the info.

>>> +export warnings c_warnings cxx_warnings base_cxxflags
>>
>> Hm, these actually don't have to be exported, do they? Seems they
>> are mostly
>> for use in SETUP.sh?
>>
>> If they need not be exported, please remove the export to avoid
>> confusion (if
>> it is needed, leave it as it is not really a problem).
>
> They do need to be exported for FINISH, as we must remove the gcc- 
> only options produced by SETUP which are incompatible with Sun cc on
> Solaris. (I'm also trying to remain compatible with Solaris sh.)

Ok, thanks for information.

(My impression was that if set in the script, they would also be available in
other scripts sourced from this first script. You can leave it, it's not that
important).

>>> +# take only #define options - the others are gcc specific
>>> +DEFS=""
>>> +for F in $debug_cflags ; do
>>> +	expr "$F" : "^-D" && DEFS="$DEFS $F"
>>> +done
>>> +debug_cflags="-O0 -g $DEFS"
>>
>> Hm. You should not really have to do this ;-). But I agree with
>> this solution
>> for now.
>
> Sure, it's ugly. I wish I could think of a better way myself.

Well, the solution would be that SETUP.sh should not put gcc-specific flags in
$debug_cflags! But fixing that should not be a blocker for your patch, I think.

>>> === modified file 'storage/innobase/include/univ.i'
>>> --- a/storage/innobase/include/univ.i	2008-03-27 01:40:45 +0000
>>> +++ b/storage/innobase/include/univ.i	2009-05-04 14:54:10 +0000
>>> @@ -58,7 +58,7 @@ of the 32-bit x86 assembler in mutex ope
>>>  /* We only try to do explicit inlining of functions with gcc and
>>>  Microsoft Visual C++ */
>>>
>>> -# if !defined(__GNUC__)
>>> +# if !defined(__GNUC__) && !defined(__SUNPRO_C)
>>>  #  undef  UNIV_MUST_NOT_INLINE			/* Remove compiler warning */
>>>  #  define UNIV_MUST_NOT_INLINE
>>>  # endif
>>>
>>
>> What is the purpose of this change?
>>
>> I am wondering if this change is unintentional (if so it should be
>> removed)?
>>
>> Or if this is really what you intended,
>
> This (along with some of the compiler options) are performance
> enhancements which have been benchmarked by other builders on
> Solaris. It is not strictly required for portability, so it could be
> removed, but you are very likely to want it for InnoDB. It merely
> enables inlining in Sun CC, prevented by the vanilla sources, but
> normally done by GCC on other platforms.

Ok, thanks for information.

I think this sounds good. So I suggest to keep it (since you tested that it
works, it seems a good change). Just mention it in the commit comment, and fix
the comment above the change (which is apparently already wrong since it
mentions Visual C++). I suggest this:

/*
  Enable explicit inlining of functions only for compilers known to support
  it.
*/

 - Kristian.