<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<font size="4"><font face="monospace">Some review comments on your
patch:<br>
<br>
- I'd prefer for all of the static methods in Classfile to go
away, though I can imagine choosing to leave the most basic
version (no options) of build/parse around as conveniences. If
we leave these, the Javadoc should make it clear that these are
equivalent to Context.of().parse(...). <br>
- I am OK with calling the context class "Classfile" for the
time being; this makes the diff easier to follow as it makes it
clear that the static methods are being promoted to instance
methods, and we can discuss name later. <br>
- I see you cache the Options in
BufWriterImpl/BufferedCodeBuilder/etc, but we should probably be
caching the full context (impl) object instead, and fetching
options out of the context. <br>
- (Syntax nit) Some enum options are fully descriptive of what
they are (DROP_DEBUG_ELEMENTS), and some are implicit
(ALWAYS_GENERATE -- generate what?). I think I would prefer to
err on the side of fully descriptive (and people can use static
imports to drop the qualifier if they like), but we should be
consistent. There should be a convention of what the default is
(like "first is always default") for options that are enums. <br>
<br>
</font></font><br>
<div class="moz-cite-prefix">On 5/26/2023 11:18 AM, Adam Sotona
wrote:<br>
</div>
<blockquote type="cite" cite="mid:CY4PR1001MB2150855058CA47CCA8532FE58C479@CY4PR1001MB2150.namprd10.prod.outlook.com">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
font-size:10.0pt;
font-family:"Calibri",sans-serif;}a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}div.WordSection1
{page:WordSection1;}</style>
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US">This is a raw prototype of Classfile.Context
implementation and the new options:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US"><a href="https://github.com/openjdk/jdk/pull/14180/files" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jdk/pull/14180/files</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US"><a href="https://github.com/openjdk/jdk/blob/4456ca78bc67138930dc313f71aba58ef1cf5f13/src/java.base/share/classes/jdk/internal/classfile/Classfile.java" moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/openjdk/jdk/blob/4456ca78bc67138930dc313f71aba58ef1cf5f13/src/java.base/share/classes/jdk/internal/classfile/Classfile.java</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US">It is far from finished, however before going
further I would appreciate your comments (mainly on options
naming).<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US">Adam<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #B5C4DF
1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">Brian
Goetz <a class="moz-txt-link-rfc2396E" href="mailto:brian.goetz@oracle.com"><brian.goetz@oracle.com></a><br>
<b>Date: </b>Thursday, 25 May 2023 20:31<br>
<b>To: </b>Adam Sotona <a class="moz-txt-link-rfc2396E" href="mailto:adam.sotona@oracle.com"><adam.sotona@oracle.com></a>,
<a class="moz-txt-link-abbreviated" href="mailto:liangchenblue@gmail.com">liangchenblue@gmail.com</a> <a class="moz-txt-link-rfc2396E" href="mailto:liangchenblue@gmail.com"><liangchenblue@gmail.com></a>,
classfile-api-dev <a class="moz-txt-link-rfc2396E" href="mailto:classfile-api-dev@openjdk.org"><classfile-api-dev@openjdk.org></a><br>
<b>Subject: </b>Re: Planned features of a classfile
context object<o:p></o:p></span></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:13.5pt;font-family:"Courier
New",serif">A model created within a context can retain
a reference to its creating context. (We do a lot of this
sort of thing already, where MethodModel retains a reference
to ClassModel, and where CPBuilder retains a reference to
Options.) So for operations that require a context, a model
derived from a classfile can use the context that created
it.<br>
<br>
Methods like parse/build/transform will be moved to the
classfile context object.
</span><span style="font-size:11.0pt"><o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On
5/25/2023 2:02 PM, Adam Sotona wrote:<o:p></o:p></span></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US" lang="EN-US">I’m trying to figure out how the context will
be passed down to the models (if it suppose to be detached
from the models).</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US">Do we plan to enforce another CC argument to
all models methods requiring context?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US">Or do we plan to strip down all
context-sensitive methods from all models and attach them
to a kind of context-wrappers?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US">For example
codeModel.forEach(Classfile.Context.of(…), …) or
Classfile.Context.of(…).forEach(codeModel, …) ?</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt" lang="EN-US">Adam</span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"><o:p> </o:p></span></p>
</div>
</blockquote>
<br>
</body>
</html>