<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<font size="4"><font face="monospace">Thanks for gathering this
list. Some quick thoughts inline.</font></font><br>
<br>
<div class="moz-cite-prefix">On 6/28/2023 2:32 AM, - wrote:<br>
</div>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">Hello, I just looked over the Classfile API and
found a few design issues, aside from simple typos addressed in <a href="https://github.com/openjdk/jdk/pull/14686" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jdk/pull/14686</a>:<br>
<div><br>
</div>
<div>Specific issues:</div>
<div>1. LocalVariable and LocalVariableType: They are missing
factory methods. In addition, they shouldn't have
writeTo(BufWriter) method exposed in the public API.</div>
</div>
</blockquote>
<br>
I see two factory methods in LocalVariable and the same in
LocalVariableType, both called "of". Are there other versions
missing? <br>
<br>
But I do agree that the inclusion of `writeTo(BufWriter)` is
implementation that should be encapsulated. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>2.
ConstantPoolBuilder::constantValueEntry/loadableConstantEntry/annotationConstantValueEntry
should accept the 4 subword types: Byte, Boolean, Char, Short,
which should simply map to int entries.</div>
</div>
</blockquote>
<br>
If we think this is needed, we'll need an overload for
constantValueEntry(Object), since Byte and friends are not
ConstantDesc, and there is unfortunately no common supertype for the
wrapper classes. If you have a sharp type in hand, you can also use
intEntry for Byte/Char/Short. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>3. BufWriter::asByteBuffer is weird: it is a write-allowed
buffer that can modify the buf until the old array is
discarded when the backing buf is expanded. Currently used by
stack map generation and stack counting, we should probably
refit this to be user-safe or make this an internal API.</div>
</div>
</blockquote>
<br>
Yes, this seems to have been added in to support the stack map
generator. I agree it should move internal and/or be refitted.<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>4. FieldRefEntry, MethodRefEntry, InterfaceMethodRefEntry,
ConstantDynamicEntry, InvokeDynamicEntry should have
typeSymbol() to easily access the shared field and method type
symbols.</div>
</div>
</blockquote>
<br>
Agree<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>5. AttributeMapper::validSince should be removed, as
mentioned by Adam a while ago; it's unused and its
implementations are sometimes incorrect.</div>
</div>
</blockquote>
<br>
I have mixed feelings about this. The JVMS has a table of when each
attribute was defined, and this is a sensible and authoritative
place to put this information. On the other hand, it is not
enforced when writing classfiles, and for attributes that are not
defined by JVMS, this quantity is basically undefined. <br>
<br>
Perhaps it should be renamed to something that doesn't imply
validation, such as "introducedInVersion"?<br>
<br>
If we're to remove it, we should ask a similar question about
allowMultiple(), though we do enforce this one and it seems useful
as a way of catching silly errors. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>General issues:</div>
<div>1. What about the attributes in Code? Currently, attributes
in code aren't delivered in Streaming besides the few
hardcoded ones, which means that custom attributes will be
lost after a code model undergoes an accept-all code
transform. Is that the intended behavior?</div>
</div>
</blockquote>
<br>
Not the intended behavior. The definition of CodeElement says:<br>
<br>
public sealed interface CodeElement extends ClassfileElement<br>
permits Instruction, PseudoInstruction,<br>
CustomAttribute,
RuntimeVisibleTypeAnnotationsAttribute,
RuntimeInvisibleTypeAnnotationsAttribute,<br>
StackMapTableAttribute {<br>
}<br>
<br>
so CustomAttribute should definitely be expected by consumers. We
eat a number of other attributes, such as line numbers, stack map,
etc, presenting their contents as pseudo-instructions. And we
definitely eat stackmaps (similar to how we treat
BootstrapMethodsTable as part of the CP, not an attribute.) But the
residue of attributes (which is usually empty) should be delivered
as custom attributes. I suspect we never noticed because so few
classfile have these. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div> 1.1. If CodeElement is only for streamable elements,
then StackMapTableAttribute shouldn't be a CodeElement, as
it's never delivered in code streams.</div>
</div>
</blockquote>
<br>
Yes, probably true. I think we could definitely do better at making
it clear which attributes are considered "part of something else",
and under what conditions they will show up anyway (e.g., if you ask
for CodeModel::attributes you will see the stack map table.) Not
clear whether the way to do this is with code (such as a
NotHandledAsElement supertype) or a section of the user's guide
(what user's guide?) about attributes.<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>2. For nullable references, currently Classfile API accepts
an Optional<Xxx> argument instead of a nullable
argument. Should we switch those to nullable arguments
instead, like parameter name for MethodParameterInfo?</div>
</div>
</blockquote>
<br>
Is there a third choice? :) Both are kind of sad. <br>
<br>
We should certainly be consistent. Thankfully, I think there are
relatively few cases here to handle? <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE1qPWqtXvvRAbBOtQ4A-hP4qi-rnm061rOoxYsy5dDo4w@mail.gmail.com">
<div dir="ltr">
<div>3. We can add @sealedGraph tag to sealed interfaces in the
Classfile API, which will render an svg image of the sealed
class hierarchy (exists in classes like j.l.i.CallSite
already)</div>
</div>
</blockquote>
<br>
For types where all the subtypes are private (and there are a lot of
these), naming the subtypes is probably just teasing the user. Does
the SVG renderer remove non-public permitted subtypes? Some of the
public types permit both public and private subtypes, which would be
confusing. <br>
<br>
Cheers,<br>
-Brian<br>
<br>
</body>
</html>