RFR: 8307312: Replace "int which" with "int cp_index" in constantPool

[David Holmes]

On Tue, 25 Jul 2023 20:20:33 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> Many accessors in the constant pool take an argument "int which" that is meant to represent an ambiguous index. Despite this, several methods in the API use "int which" when the argument is exclusively a constant pool index. This patch aims to rename all of these instances to "int cp_index" in order to be more clear and to distinguish methods that take constant pool indices and methods that use derived indices.
> 
> The callers have been updated to use more clear naming as well. Verified with tier 1-5 tests.

The basic `which` change is good and useful, but I don't think most of the other renamings were really necessary. It is hard to maintain a convention that an index into to the CP must always be called `cp_index` - the `cp` is often redundant given the context.

Please double-check all comments where parameter names have changed to ensure they refer to the new names.

Thanks.

src/hotspot/share/oops/constantPool.cpp line 237:

> 235:   int len = length();
> 236:   int num_klasses = 0;
> 237:   for (int cp_index = 1; cp_index <len; cp_index++) {

I don't think the use of `i` as a simple loop variable caused any problem in understanding here.

src/hotspot/share/oops/constantPool.cpp line 456:

> 454: }
> 455: 
> 456: void ConstantPool::string_at_put(int obj_index, oop str) {

Should `obj_index` be `cp_index` here?

src/hotspot/share/oops/constantPool.cpp line 945:

> 943: // called to create oops from constants to use in arguments for invokedynamic
> 944: oop ConstantPool::resolve_constant_at_impl(const constantPoolHandle& this_cp,
> 945:                                            int cp_index, int cache_index,

Not sure that we really need to always say `cp_index` given the context. Changing `which` is good, but `index` was perfectly fine IMO.

src/hotspot/share/oops/constantPool.cpp line 1615:

> 1613: // to the constant pool to_cp's entries starting at to_i. A total of
> 1614: // (end_i - start_i) + 1 entries are copied.
> 1615: void ConstantPool::copy_cp_to_impl(const constantPoolHandle& from_cp, int start_cpi, int end_cpi,

The comment needs updating now the parameter names have changed.

Again I don't think there was any need to change the names in these cases.

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15027#pullrequestreview-1553712856
PR Review Comment: https://git.openjdk.org/jdk/pull/15027#discussion_r1278624771
PR Review Comment: https://git.openjdk.org/jdk/pull/15027#discussion_r1278624805
PR Review Comment: https://git.openjdk.org/jdk/pull/15027#discussion_r1278625974
PR Review Comment: https://git.openjdk.org/jdk/pull/15027#discussion_r1278625517