RFR: 8287089: G1: Change const <type>* name to <type>* const name in arguments

[Kim Barrett]

On Fri, 20 May 2022 16:02:58 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

> Hi all,
> 
>   can I have reviews for this change that fixes as few `const` modifiers, making them more consistent; mostly changing `const <type>* something` to `<type>* const something` as in many cases the pointer value should be constant, not the thing that is pointed to.
> 
> I also considered changing `const <typedef> something` where `typedef` is a pointer type (e.g. `oop` is `oopDesc*`), but `const oop something` is equal to `oop const something` (it only concerns `oop` in our code) to minimize the changes. If you think we should change these argument declarations to `oop const something` to be similar to other code, I can do that.
> 
> This is a preparatory change for [JDK-8210708](https://bugs.openjdk.java.net/browse/JDK-8210708) where some of the affected method get another parameter, and it looks weird to have e.g. `block_start(const HeapWord* addr, HeapWord* const pb)`, i.e. two parameters where although both are meant to be constant pointers, they have different `const`s.
> 
> Testing: local compilation, GHA
> 
> Thanks,
>   Thomas

I only looked at a couple of files before stopping to question the whole of
this change. Many of these look wrong to me.

> can I have reviews for this change that fixes as few `const` modifiers,
> making them more consistent; mostly changing `const <type>* something` to
> `<type>* const something` as in many cases the pointer value should be
> constant, not the thing that is pointed to.

I think these changes are incorrect.

Changing `const T*` to `T* const` is a semantic change.  The former is
"pointer to constant T" while the latter is "constant pointer to T".  In all
the places I looked at so far, that change should not be made.

Also note that in a (non-definitional) declaration, a top-level CV qualifier
is ignored. So a parameter of type `T* const` is the same as a parameter of
type `T*`. The const only has impact in a definition, when it declares the
variable (as opposed to its value) as constant.

> I also considered changing `const <typedef> something` where `typedef` is a
> pointer type (e.g. `oop` is `oopDesc*`), but `const oop something` is equal to
> `oop const something` (it only concerns `oop` in our code) to minimize the
> changes. If you think we should change these argument declarations to `oop
> const something` to be similar to other code, I can do that.

`const oop` is "constant pointer to (non-constant) oopDesc*`, and is
equivalent to `oop const`.  I don't think making the described change should
be done.

> This is a preparatory change for
> [JDK-8210708](https://bugs.openjdk.java.net/browse/JDK-8210708) where some of
> the affected method get another parameter, and it looks weird to have e.g.
> `block_start(const HeapWord* addr, HeapWord* const pb)`, i.e. two parameters
> where although both are meant to be constant pointers, they have different
> `const`s.

I don't know what "pb" is in that signature, but the const qualifier for it isn't useful.

src/hotspot/share/gc/g1/g1BlockOffsetTable.hpp line 124:

> 122:   void set_remainder_to_point_to_start_incl(size_t start, size_t end);
> 123: 
> 124:   inline size_t block_size(HeapWord* const p) const;

This looks wrong.  I don't think this operation modifies (nor should it) the contents of p, so the original declaration is correct.  The two are not equivalent (`const T*` is "pointer to constant T", while `T* const` is "constant pointer to T").  Also, a top-level `const` qualifier in a (non-definitional) declaration is ignored.  Similarly for the other changes in this file.

-------------

Changes requested by kbarrett (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8815