> On Aug 16, 2019, at 4:20 AM, Stefan Karlsson <stefan.karls...@oracle.com>
> wrote:
>
> On 2019-08-16 00:59, Kim Barrett wrote:
>> src/hotspot/share/oops/markOop.hpp
>> 109 template<typename T> operator T();
>> My mistake in the earlier review comment. Function should be const
>> qualified, e.g. that should be
>> template<typename T> operator T() const;
>
> I added this after one of our earlier discussions. However, I don't think we
> need it (const or not). We already get sensible compiler errors without this
> function when we try to cast markWords to something else:
>
> void* p0 = m;
> void* p1 = (void*)m;
> int i0 = m;
> int i1 = (int)m;
>
> [… various errors …]
You’re right. It seems I need to refresh my recollection of what the valid
conversions are.
> The poisoned constructor seems to be unnecessary as well, now that we have
> simplified markWord. Without it, I get appropriate error messages when I try
> to create a markWord from a pointer:
>
> error: invalid conversion from ‘void*’ to ‘uintptr_t’ {aka ‘long unsigned
> int’} [-fpermissive]
> markWord p((void*)0x111);
> ^~~~~~~~~~~~
> note: initializing argument 1 of ‘markWord::markWord(uintptr_t)’
> explicit markWord(uintptr_t value) : _value(value) { }
I no longer recall why that one was even suggested. But you are right that it
isn’t needed.
> I've removed both of these.
Good.
> […]
>
> I'll leave the rest of the comments below for follow-up RFEs.
OK.
> This is the last few cleanups:
> http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04.delta/
> http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04/
Looks good.
> I ran extended testing on .03 (tier1-7 on linux), checked the markWord
> functions were inlined, and checked that the generated code for
> G1ParScanThreadState::copy_to_survivor_space was the same before and after
> the patch. So I intend to run tier1 testing on .04 and then push this patch.
>
Thanks for checking the generated code. It’s what we expected, but compilers
are sometimes surprising.
