<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 6/28/2023 11:02 AM, - wrote:<br>
</div>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">And for the rest, here are my responses, inlined: <br>
<div class="gmail_quote">Specific issues:
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<blockquote type="cite">
<div dir="ltr">
<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? </div>
</blockquote>
<div>Sorry, I meant classfile.attribute.LocalVariableInfo and
LocalVariableTypeInfo.</div>
</div>
</div>
</blockquote>
<br>
Since these are purely internal classes, I think instantiating
through constructors is fine here.<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote"><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> <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>
</div>
</blockquote>
<div>I think we will move this to internal for now, as its
behavior as a public API is up to debate. We can always add
it in the future shall we need it. <br>
</div>
</div>
</div>
</blockquote>
<br>
This works for me.<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> <br>
<br>
<blockquote type="cite">
<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. </div>
</blockquote>
<div>Introduction version is more like a Classfile
version-based toggle to treat an attribute too new as
unknown (default behavior per JVMS). Maybe users want more
fine-grained control to parse attributes based on class
reader context, which I think may be better done by allowing
mapper to return Optional.empty/null for readAttribute; and
Classfile API may emit an UnknownAttribute instead based on
options.</div>
</div>
</div>
</blockquote>
<br>
Having options to determine whether to filter inapplicable
attributes seems a configuration mess for little value. I'm OK with
removing validSince(), since we are unlikely to act on it
substantially. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> <br>
<br>
<blockquote type="cite">
<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. </div>
</blockquote>
<div>In addition, if we do stream custom attributes, we might
declare UnknownAttribute a subtype of CodeElement and stream
them as well, if the process unknown attribute option
allows. <br>
</div>
</div>
</div>
</blockquote>
<br>
Yes, that seems right. <br>
<br>
Separately: we need better documentation for attribute handing.
This could go in a section of the package Javadoc, but there are a
number of conceptual things to capture: how some attributes are
considered part of some other classfile element, ho those attributes
are presented as elements, how to use AttributeMapper, what happens
with unknown attributes, compability considerations when adapting
classfiles with unknown attributes, the horrible special story of
the type annotation attributes, etc. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> <br>
<br>
<blockquote type="cite">
<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>
</div>
</blockquote>
<div>The ClassfileElement is currently only restricting
streaming; attributes themselves can appear on any
AttributedElement without type restriction, and attribute
mappers work across all contexts as well. </div>
</div>
</div>
</blockquote>
<br>
Yes, this is how it was intended -- the attributes bytes are always
available if you ask the right way, but the element view shows you a
logical, if not physical, view of the classfile. <br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote"><br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> 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? </div>
</blockquote>
<div>It's said that Optional wrapper would bring extra
overhead even when value types drop, especially if the
wrapped type could have been inlined. Code wise, it does
look somewhat verbose (compared to the traditional
null-based approach used by familiar ASM), so it's more like
a personal habit thing, and I slightly favor nullable due to
IDE warnings and preexisting ASM usage. I think we can find
most use cases already by searching for Optional in
Classfile API.</div>
</div>
</div>
</blockquote>
<br>
OK, let's make a list of both nullable and Optional parameters (this
is a useful doc item to have anyway), and make the least bad choice
we can given the context.<br>
<br>
<blockquote type="cite" cite="mid:CABe8uE3+pfyOk0zxOxPyxj3UjPxJGPv=BQsMSj3AujD_Ow9+=w@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div> <br>
<blockquote type="cite">
<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. </div>
</blockquote>
<div>It does filter and only render types in public API: see
the FFI API's MemoryLayout example: <a href="https://urldefense.com/v3/__https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/MemoryLayout-sealed-graph.svg__;!!ACWV5N9M2RV99hQ!PA1jju5DwgMm1CKiRbiacC3UCYN1J8nVqRQqAdY29KdBfWS1MjQVL05xA50Dd81m77aRsG0F5i7mL6wtlHYiBPNz$" moz-do-not-send="true">https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/MemoryLayout-sealed-graph.svg</a></div>
</div>
</div>
</blockquote>
<br>
OK, so I think we can apply this to interfaces that have any public
subtypes.<br>
<br>
<blockquote type="cite">
<div>Again, thank you for your time and this detailed review!</div>
</blockquote>
<br>
Thanks for your diligent help with this project, your efforts are
definitely improving both the implementation and API.<br>
<br>
Cheers,<br>
-Brian<br>
<br>
</body>
</html>