Fix array constructor for Object Lists #17503
Conversation
`new[]()` not supported on older compilers. Instead, just calling `new()` in a loop for every element of the array. Signed-off-by: Aleksandar Micic <Aleksandar_Micic@ca.ibm.com>
|
Jenkins compile pLinux jdk8 |
|
real testing has been done in the internal build http://vmfarm.rtp.raleigh.ibm.com/build_info.php?build_id=52180 |
| for (uintptr_t index = 0; index < arrayElements; index++) { | ||
| new(&continuationObjectLists[index]) MM_ContinuationObjectList(); |
There was a problem hiding this comment.
Paretheses should not be used after the class name (and there should be a space after new):
new (&continuationObjectLists[index]) MM_ContinuationObjectList;Same comment applies to other files included in this change.
There was a problem hiding this comment.
I personally like space between new and open bracket, but no-space is already very much adopted in J9 GC code (there are dozens of examples without space and barely any with).
As far as I know, the oval brackets are needed at the end - it's an indication that (in this case default) constructor should be invoked.
There was a problem hiding this comment.
They are not needed; their presence represents a possible ambiguity.
Here we want the compiler to invoke the (user-defined) default constructor.
Using parentheses is a request for zero-initialization instead.
There's a discussion of this at https://stackoverflow.com/questions/620137/do-the-parentheses-after-the-type-name-make-a-difference-with-new.
There was a problem hiding this comment.
Thanks for the reference to the discussion. But from what I can see, our case resembles to 'C' class (non-POD, with user defined ctor), and in both older/newer versions of compilers, calling it with '()', will call default ctor, what we want. I don't understand from where you get that it will do zero-init?
BTW, we have this pattern at a couple of 100 of spots, already. If we are indeed calling it wrong we are really lucky (that default ctor is just pretty much zero-init).
There was a problem hiding this comment.
Perhaps an analogy will help. You wouldn't write
MM_ContinuationObjectList local();to declare an initialize a local variable (the snippet declares a function which returns MM_ContinuationObjectList). Instead you would say
MM_ContinuationObjectList local;Along the same lines, I'm suggesting we should write new (where) T instead of new (where) T(). I believe there's potential to create ambiguity when a suitable call operator (operator()()) is available (although I haven't yet had luck constructing a concrete example).
There was a problem hiding this comment.
I somewhat buy that analogy and from that perspective, it would more consistent to do new (where) T instead of new (where) T(). But I don't see the reason to use the former to reduce/remove ambiguity. We probably always have default ctor (so it will be called over zero-init), and even if we don't I don't see a problem (it will call zero-init, what was likely the intention).
In presence of overloaded operator()(), yes, there could be ambiguity problems, but I don't want to go there. We have never overloaded it, and pretty sure never will. It's just such an extreme operator to overload.
So, do we want to go and change 10s of spots just for this analogy argument? I'd be ok if we are writing the code from scratch, but now, it's a tougher call. I'll think about it more...
There was a problem hiding this comment.
It may not be worth fixing the existing instances of this, but I think it's something to keep in mind going forward: let's not make it worse.
new[]()not supported on older compilers. Instead, just callingnew()in a loop for every element of the array.