> 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?
OK. >> Instead of passing pointers to classFileParser's new attributes fields >> (_synthetic_flag, _sourcefile, ...) as arguments add accessors >> functions which set these fields. > > 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 For parse_fields() references for local variables are passed. It is different from passing references to classFileParser's fields which are accessible inside methods. > 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. > > But I'll change it if you insist. Please, change. Also put final klass settings "Fill in field values from parse_classfile_attributes" in a separate ClassFileParser method. Thanks, Vladimir John Rose wrote: > On Jul 11, 2012, at 12:01 AM, Michael Haupt wrote: > >> @@ -1636,16 +1648,163 @@ >> 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). >> >> @@ -2560,10 +2727,11 @@ >> TempNewSymbol sde_symbol is never used. > > Fixed; thanks. > > > On Jul 11, 2012, at 2:35 PM, Vladimir Kozlov wrote: > >> c1_GraphBuilder.cpp, can you remove setting bailout message for forced >> inlining? It should be done proper uniform way for C1 inlining later. > > Done. > >> I think, next assert should check (id >= _unknown && id < >> _annotation_LIMIT) instead: >> >> + void set_annotation(ID id) { >> + assert((int)id > 0 && (int)id < BitsPerInt, "oob"); > > Good. I added this to the constructor > assert((int)_annotation_LIMIT <= (int)sizeof(_annotations_present) * > BitsPerByte, ""); > >> In header file classFileParser.hpp you should not specify >> ClassFileParser:: in method parse_classfile_attributes(() declaration: >> >> ClassFileParser::ClassAnnotationCollector* parsed_annotations > > Good catch. > >> Instead of asserts in apply_to() methods we should use guarantee("not >> implemented") or something. > > Done: > + guarantee(false, "no field annotations yet"); > >> >> I don't think next should be part of these changes: >> >> +#if 0 >> + // The parsing of @Retention is for example only. > > 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? > + // For the record, here is how annotation payloads can be collected. > + // Suppose we want to capture @Retention.value. Here is how: > + //if (id == AnnotationCollector::_class_Retention) { > >> Add parenthesis around expression in next condition: >> >> + while (--nann >= 0 && index-2 + min_size <= limit) { > > Done: > + while ((--nann) >= 0 && (index-2 + min_size <= limit)) { > >> Instead of passing pointers to classFileParser's new attributes fields >> (_synthetic_flag, _sourcefile, ...) as arguments add accessors >> functions which set these fields. > > 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. > > But I'll change it if you insist. > >> I think next is typo, should be _in_method check: >> >> + case >> vmSymbols::VM_SYMBOL_ENUM_NAME(java_lang_invoke_ForceInline_signature): >> + if (_location != _in_class) break; >> + return _method_ForceInline; >> + default: break; >> + } > > Yes; thanks. > > — John _______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev