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