review request (M): 6711908: JVM needs direct access to some annotations

[Vladimir Kozlov]

 > 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