On Jul 11, 2012, at 3:47 PM, 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");
fatal would be the right one; it doesn't take a boolean.
-- Chris
>
>>
>> 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
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
