RFR: 8013393: Merge templateTable_x86 32 and 64 bit files into one file.

[serguei.spitsyn at oracle.com]

Hi Max,

A minor comment.
The testlist (nsk.jvmti.testlist) looks strange.
I'd expect at least some runtime tests to be run.

Thanks,
Serguei


On 3/3/15 1:39 PM, Max Ockner wrote:
> Hello all,
> I have made all of the changes recommended by Chris and John.  I 
> realize there are plenty of further cleanups and optimizations that I 
> could make, but I think this is nearing a good stopping point for now. 
> We can always clean it up more later. What do you think?
>
> new webrev:   cr.openjdk.java.net/~coleenp/8013393
> tested with nsk.jvmti.testlist
>
> On 3/2/2015 4:45 PM, John Rose wrote:
>> On Mar 2, 2015, at 11:46 AM, Christian Thalinger 
>> <christian.thalinger at oracle.com> wrote:
>>> That’s quite a large change (I wish we had Crucible or something to 
>>> leave inline comments).
>>>
>>> First, you have to keep the oldest copyright year (but maybe that’s 
>>> just the diff).
>>>
>>> + #ifdef _LP64
>>> + void TemplateTable::lconst(int value) {
>>>
>>> I’d prefer to have #ifdef’s inside methods and not around them.  
>>> There are a bunch; please change all of them.
> I have moved the #ifdef's to the inside of the function declarations. 
> Some of the #ifdefs could be moved far enough in that it made sense to 
> replace them with the LP64_ONLY() or NOT_LP64() tools. The remaining 
> #ifdef's are there because some large sections of code could not be 
> merged, and needed to be kept together.
>
>> I agree Christian's comments, and especially with this request.
>>
>> A few more points on small matters:
>>
>> In our source base, C++ comments usually begin with two slashes and a 
>> space (less than 10% exceptions).
>>    +//Global Register Names
>>    +//Address Computation: local variables
>>    (etc.)
>
> Fixed.
>
>> Let's use the term "amd64" only where required; x86_64 is the 
>> standard now.
>>     // No amd64 specific initialization
>
> Fixed.
>
>> More on #ifdefs:  I think there are too many; Chris has pointed out 
>> the main problem.
>>
>> It's a judgement call, but here's another example:
>>
>> +#ifdef _LP64
>> +        Register rtmp = r8;
>> +        Register rthread = r15_thread;
>> +#else
>> +        Register rtmp = rsi;
>> +        Register rthread = rcx;
>> +        __ get_thread(rcx);
>> +        __ save_bcp();
>> +#endif // _LP64
>>
>> could just as well be:
>>
>> +        Register rtmp = LP64_ONLY(r8) NOT_LP64(rsi);
>> +        Register rthread = LP64_ONLY(r15_thread) NOT_LP64(rcx);
>> +        LP64_ONLY(__ get_thread(rcx));
>> +        LP64_ONLY(__ save_bcp());
>>
>> Advantages:  Fewer lines, less duplication, less chance of merge 
>> problems.
>> Possible objection:  Ugly; but only in a different way from the other 
>> ugly.
>
> Fixed, but there are some #ifdef's left over. There are some pretty 
> large code blocks that are conditionally defined. I don't think it 
> makes sense to enclose every conditionally defined line of code in an 
> LP64_ONLY or NOT_LP64. At best you save 3 lines of code, but I feel 
> that a those 3 lines would be less distracting than having 25 
> LP64_ONLY's in a row.
>>
>> However, I admit that is a marginal example.
>>
>> — John
>
> Thanks,
> Max Ockner
>