<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@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;}
@font-face
{font-family:"Courier New \,serif";
panose-1:2 7 3 9 2 2 5 2 4 4;}
/* Style Definitions */
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;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="en-CZ" link="#0563C1" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language: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 <brian.goetz@oracle.com><br>
<b>Date: </b>Friday, 26 May 2023 23:16<br>
<b>To: </b>Adam Sotona <adam.sotona@oracle.com>, liangchenblue@gmail.com <liangchenblue@gmail.com>, classfile-api-dev <classfile-api-dev@openjdk.org><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">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.<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span lang="EN-US" style="font-size:11.0pt">OK, Classfile.of().parse(…) is much easier to find than Classfile.Context.of().parse(…), so I’ll remove the static methods.<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:13.5pt;font-family:"Courier New",serif">
<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.<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span lang="EN-US" style="font-size:11.0pt">impl.Options was the context class and now it only become implementation of Classfile.Context, I’ll rename it to ClassfileImpl.<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:13.5pt;font-family:"Courier New",serif">
<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>
<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span lang="EN-US" style="font-size:11.0pt">I’ll fix it, thanks.<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span lang="EN-US" style="font-size:11.0pt">Adam<o:p></o:p></span></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt"><o:p> </o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On 5/26/2023 11:18 AM, Adam Sotona wrote:<o:p></o:p></span></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US">This is a raw prototype of Classfile.Context implementation and the new options:</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US"><a href="https://github.com/openjdk/jdk/pull/14180/files">https://github.com/openjdk/jdk/pull/14180/files</a></span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US"><a href="https://github.com/openjdk/jdk/blob/4456ca78bc67138930dc313f71aba58ef1cf5f13/src/java.base/share/classes/jdk/internal/classfile/Classfile.java">https://github.com/openjdk/jdk/blob/4456ca78bc67138930dc313f71aba58ef1cf5f13/src/java.base/share/classes/jdk/internal/classfile/Classfile.java</a></span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US">It is far from finished, however before going further I would appreciate your comments (mainly on options naming).</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US">Adam</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language:EN-US"> </span><o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;mso-fareast-language:EN-US"> </span><o:p></o:p></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 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 href="mailto:adam.sotona@oracle.com"><adam.sotona@oracle.com></a>,
<a href="mailto:liangchenblue@gmail.com">liangchenblue@gmail.com</a> <a href="mailto:liangchenblue@gmail.com">
<liangchenblue@gmail.com></a>, classfile-api-dev <a 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</span><o:p></o:p></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><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt">On 5/25/2023 2:02 PM, Adam Sotona wrote:</span><o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;mso-fareast-language: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 lang="EN-US" style="font-size:11.0pt">Do we plan to enforce another CC argument to all models methods requiring context?</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt">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 lang="EN-US" style="font-size:11.0pt">For example codeModel.forEach(Classfile.Context.of(…), …) or Classfile.Context.of(…).forEach(codeModel, …) ?</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt"> </span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt">Adam</span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"> </span><o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span style="font-size:11.0pt"><o:p> </o:p></span></p>
</div>
</body>
</html>