On Wed, 15 Feb 2023 08:20:19 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java >> line 537: >> >>> 535: * @param <T> the type of the entry >>> 536: */ >>> 537: <T extends PoolEntry> T maybeClone(T entry); >> >> This feels a more primitive operation than the name suggests. Have you >> considered making ConstantPool extend Consumer<PoolEntry> and call this >> "accept" ? > > I'm not quite sure what exactly do you propose. > `ConstantPool` should not accept anything as it is read-only, so "accept" > would be confusing. > `ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name > might be changed to `ConstantPoolBuilder::apply`. > However there are so many "accepts" and "applies" across the API, that > reducing API verbosity to just these functional terms might significantly > decrease readability. As I read the javadoc (I have not looked at impl yet), this method can effectively be used to add an entry to the constant pool (if the entry does not yet exist - in which case existing entry is returned). After having looked at the implementation a bit, I think my assumption is correct - e.g. when calling `getfield` on a code builder, the FieldRef passed to the code builder is added to the constant pool using `maybeClone`. So, in my mind at least, this `maybeClone` is the main avenue by which new constant pool entries are added. In builders we have a `with` method - I think a `with` method would also be ok here, given what the method does (ok, there is the wrinkle that, if the entry already exists it is not added, but that seems more of an optimization to avoid duplicates). ------------- PR: https://git.openjdk.org/jdk/pull/10982
