Re: bzr commit into mysql-6.0-falcon branch (olav:2758) Bug#37622

[Olav Sandstaa]

Hi Vlad,

Thanks for reviewing and commenting on my patch. See inline for responses.

Vladislav Vaintroub wrote:
> Olav, that looks great, actually ok to push for me, but here are some
> thoughts mostly on style.
>
> do you really need cas_sparc, cas_sparc_pointer32 and cas_sparc_pointer64?
> Is it not enough to have cas_sparc32 and cas_sparc64 (the code for cas_sparc
> looks suspiciously similar to _pointer32 variation
>   

I agree that the code that implements the cas_sparc and _pointer32 
variant is identical. But the function declaration (see the argument 
list) for these two functions are not identical. And I think I must have 
a one-to-one correspondence between function declarations and 
implementations when using Sun Studio's inline assembly templates. (I 
could probably get around this by using some casting of arguments or 
using the preprocessor to do some renaming, but I think it is clearer 
the way it is now). Anyway, the reason for two identical implementations 
is that they implements two different functions :-)

>> +inline int cas_pointer_sparc(volatile void **target, void *compare,
>> +                             void *exchange) {
>> +  if (sizeof(void*) == 4) {
>> +    return cas_pointer_sparc32(target, compare, exchange);
>> +  }
>> +  else {
>> +    return cas_pointer_sparc64(target, compare, exchange);
>> +  }
>> +}
>>     
>
> For Falcon, "Yet Another True Bracing Style" is used, the one you've got
> here reveals your  roots too obviously;)
> Also one can save 4 braces here altogether, and "else" as well
>   

Thanks, I will update this to follow the Falcon coding style. Sorry for 
not having done that already but old habbits are hard to get rid of - 
particularly when you start a new source file just for experimenting and 
then end up including it in the final patch.

> Inline function for pointer is fine (hopefully compiler will not really
> generate compare and jump instructions ;)), but would not that be more
> natural to have
>
> #if SUNSTUDIO_SPECIFIC_64_BIT_PREDEFINED_MACRO
> #define cas_pointer_sparc cas64
> #else
> #define cas_pointer_sparc cas32
> #endif
>   

This was a result of my philosophy when programming C++: "prefer to use 
the compiler and the C++ language to the C-preprosessor" (as long as 
this does not harm performance). I considered using similar macros as 
you suggested but ended up with writing this as the inline function as 
it is. It also has the added benefit that looking at the code in a 
compiler or call stack reveals the correct function names (at least in 
debug mode). Functionality and performance of these two implementations 
should be the same. I am really not sure which implementation I prefer.

> I do like that you have all sparc stuff  in an extra header, maybe we should
> apply this everywhere else for consistency? ( that is ,create a bunch of
> headers msvc-generic, gcc-x86, gcc-ppc, solaris-intrinsics, solaris-sparc
> (we don't yet use gcc-builtins, but could do so it in the future )
>
>   

Thanks. I agree that Interlock.h might look a bit cleaner if we had 
moved at least the assembly code out if it and into separate files. The 
main reason I did that was that I anyway had to introduce a new file for 
the Sun Studio inline assembly and then it was natural to make a 
corresponding interface file for it.

Thanks for the comments, Vlad!

Regards,
Olav