RFR: 8013393: Merge templateTable_x86 32 and 64 bit files into	one file.
    John Rose 
    john.r.rose at oracle.com
       
    Mon Mar  2 21:45:16 UTC 2015
    
    
  
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 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.)
Let's use the term "amd64" only where required; x86_64 is the standard now.
   // No amd64 specific initialization
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.
However, I admit that is a marginal example.
— John
    
    
More information about the hotspot-dev
mailing list