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

[John Rose]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20120711/742326f9/attachment.html