<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jul 11, 2012, at 12:01 AM, Michael Haupt wrote:</div><div><div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div>@@ -1636,16 +1648,163 @@</div><div>The code for parsing @Retention deserves a comment highlighting that it is about parsing an annotation with payload (none of the annotations introduced by our work do this).</div><div><br></div><div>@@ -2560,10 +2727,11 @@</div><div>TempNewSymbol sde_symbol is never used.</div></span></blockquote></div><br></div><div>Fixed; thanks.</div><div><br></div><div><br></div><div>On Jul 11, 2012, at 2:35 PM, Vladimir Kozlov wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>c1_GraphBuilder.cpp, can you remove setting bailout message for forced inlining? It should be done proper uniform way for C1 inlining later.</div></blockquote><div><br></div><div>Done.</div><br><blockquote type="cite"><div>I think, next assert should check (id >= _unknown && id < _annotation_LIMIT) instead:<br><br>+ void set_annotation(ID id) {<br>+ assert((int)id > 0 && (int)id < BitsPerInt, "oob");<br></div></blockquote><div><br></div>Good. I added this to the constructor</div><div> assert((int)_annotation_LIMIT <= (int)sizeof(_annotations_present) * BitsPerByte, "");</div><div><br><blockquote type="cite"><div>In header file classFileParser.hpp you should not specify ClassFileParser:: in method parse_classfile_attributes(() declaration:<br><br>ClassFileParser::ClassAnnotationCollector* parsed_annotations<br></div></blockquote><div><br></div><div>Good catch.</div><br><blockquote type="cite"><div>Instead of asserts in apply_to() methods we should use guarantee("not implemented") or something.<br></div></blockquote><div><br></div>Done:</div><div> + guarantee(false, "no field annotations yet");</div><div><br><blockquote type="cite"><div><br><font class="Apple-style-span" color="#000000"></font>I don't think next should be part of these changes:<br><br>+#if 0<br>+ // The parsing of @Retention is for example only.<br></div></blockquote><div><br></div><div>Yes, Michael asked about that too. The point is to show how annotation payloads can be processed. Somebody will need it at some point. I'll make it into a comment; is that OK?</div><div><div>+ // For the record, here is how annotation payloads can be collected.</div><div>+ // Suppose we want to capture @Retention.value. Here is how:</div><div>+ //if (id == AnnotationCollector::_class_Retention) {</div></div><div><br></div><blockquote type="cite"><div>Add parenthesis around expression in next condition:<br><br>+ while (--nann >= 0 && index-2 + min_size <= limit) {<br></div></blockquote><div><br></div><div>Done:</div><div> + while ((--nann) >= 0 && (index-2 + min_size <= limit)) {</div><div><br></div><blockquote type="cite"><div>Instead of passing pointers to classFileParser's new attributes fields (_synthetic_flag, _sourcefile, ...) as arguments add accessors functions which set these fields.<br></div></blockquote><div><br></div><div>The pointer passing is following the pattern already present for parsing other constructs. parse_fields is the clearest example. The parsed information is returned by reference. I could do as you suggest, but it would work only for top-level class attributes, so there would still be a mix of styles. My thought is to make the style more uniform by relying on the return-by-reference pattern.</div><div><br></div><div>But I'll change it if you insist.</div><br><blockquote type="cite"><div>I think next is typo, should be _in_method check:<br><br>+ case vmSymbols::VM_SYMBOL_ENUM_NAME(java_lang_invoke_ForceInline_signature):<br>+ if (_location != _in_class) break;<br>+ return _method_ForceInline;<br>+ default: break;<br>+ }<br></div></blockquote><br></div><div><div><div><font class="Apple-style-span" color="#000000">Yes; thanks.</font></div></div></div><div><font class="Apple-style-span" color="#000000"><br></font></div><div><font class="Apple-style-span" color="#000000">— John</font></div></body></html>