RFR (S) 8165929: Constify arguments of Copy methods

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Feb 8 19:14:47 UTC 2018


Thank you for the review.

On 2/8/18 1:56 PM, Kim Barrett wrote:
>> On Feb 7, 2018, at 10:35 AM, coleen.phillimore at oracle.com wrote:
>>
>> I actually didn't see any calls where const was cast away, and I wasn't going to change them anyway.  Most of the calls to the Copy functions cast both arguments to HeapWord when there's casting involved.  Anyway, this should help for future uses and cleanups. This is very limited and hopefully a very boring review.
>>
>> I wasn't able to test PPC and AARCH64,
> or S390 (see second item below)
>
>> but tested Oracle's current and former supported platforms with this change (note 32 bit arm reverses arguments to one of the _Copy functions).   I compiled with Zero but can't run Zero for some reason unrelated.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8165929.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8165929
>>
>> Thanks,
>> Coleen
> Looks good.  A small number of minor issues, for which I don't need a
> new webrev.
>
> ------------------------------------------------------------------------------
> src/hotspot/cpu/s390/copy_s390.hpp
>   108 static bool has_destructive_overlap(const char* from, char* to, size_t byte_count) {
>
> Pre-existing:
>
> I think this function should take [const] void* arguments and locally
> convert for arithmetic, rather than forcing nearly all callers to cast
> arguments.  But that's not a problem with this changeset.

Yes, I agree.  I'm not going to fix this.
>
> ------------------------------------------------------------------------------
> src/hotspot/cpu/s390/copy_s390.hpp
> 1030 static void pd_arrayof_conjoint_oops(HeapWord* from, HeapWord* to, size_t count) {
>
> Missed a const qualifier here.
>
> ------------------------------------------------------------------------------
> src/hotspot/cpu/sparc/copy_sparc.hpp
>   118   pd_conjoint_oops_atomic((oop*)from, (oop*)to, count);
>   143   pd_conjoint_jshorts_atomic((jshort*)from, (jshort*)to, count);
>   147   pd_conjoint_jints_atomic((jint*)from, (jint*)to, count);
>   151   pd_conjoint_jlongs_atomic((jlong*)from, (jlong*)to, count);
>   155   pd_conjoint_oops_atomic((oop*)from, (oop*)to, count);
>
> For consistency, these casts of from ought to retain const.
>
> ------------------------------------------------------------------------------
> src/hotspot/os_cpu/linux_x86/copy_linux_x86.inline.hpp
>   288   pd_conjoint_jints_atomic((jint*)from, (jint*)to, count);
>   296   pd_conjoint_jlongs_atomic((jlong*)from, (jlong*)to, count);
>   305   pd_conjoint_oops_atomic((oop*)from, (oop*)to, count);
>
> For consistency, these casts of from ought to retain const.
>
> ------------------------------------------------------------------------------
> src/hotspot/os_cpu/windows_x86/copy_windows_x86.inline.hpp
>   122   pd_conjoint_oops_atomic((oop*)from, (oop*)to, count);
>
> For consistency, these casts of from ought to retain const.
>
> ------------------------------------------------------------------------------
>

I missed these.  Thank you for spotting them! I fixed these now and will 
rerun through testing just to make sure.

Thanks!
Coleen


More information about the hotspot-runtime-dev mailing list