RFR(S) 7172226: HotSpot fails to build with GCC 4.7 because of stricter c++ argument dependent lookup

[Bengt Rutisson]

Hi Mikael,

On 2012-05-29 17:07, Mikael Gerdin wrote:
> (changed the subject to clarify that there's a code review hidden in 
> here)
>
> As stated earlier in this thread GCC 4.7 has become more strict when 
> doing name lookup in template classes.
> I have a patch that will fix the build
> http://cr.openjdk.java.net/~mgerdin/7172226/webrev.0/

Thanks for fixing this. I think it looks good!

> but I'm not sure about the style, the code already contains some 
> wrapper functions which look like they tried to solve the same problem.
>
> Does anyone have any suggestions on how to handle this more cleanly?

There seem to be three styles of handling this:

(1) add "this->"
(2) add "BaseType<TemplateType>::"
(3) create shadowing methods that just use (1) or (2) to access the 
correct method

Right now we use all three and I think it would be nice if we can reduce 
it to one or maybe two styles. On the other hand this does not seem to 
be a very large issue in the Hotspot source in general, so I am not sure 
the guidelines need to be too strict. If we find an easy way of being 
more consistent it would be good. Otherwise I am fine with the patch as 
it is.

I kind of like (1) as it always works and it is not that intrusive. But 
when you read the code it gives you no clue why it was necessary to add 
"this->". (2) would give more information when you read the code, but it 
doesn't work for pure virtual methods. So, maybe using (3) and implement 
it using (1) is a good compromise if we make sure to add comments to the 
shadowing methods documenting why we need them.

Just my thoughts after discussing this with Mikael and Stefan a bit...

Bengt

>
> Thanks
> /Mikael
>
> On 2012-05-29 15:11, Andrew Hughes wrote:
>>
>>
>> ----- Original Message -----
>>> Andrew,
>>>
>>> I filed 7172226
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7172226 (not
>>> available yet)
>>> See below for the patch.
>>>
>>> On 2012-05-28 15:34, Andrew Hughes wrote:
>>>>
>>>>
>>>> ----- Original Message -----
>>>>> Hi Andrew,
>>>>>
>>>>> On 2012-05-28 14:57, Andrew Hughes wrote:
> (...)
>>>>>
>>>>> I've seen this when I tried building with GCC 4.7 as well.
>>>>> This problem comes from 9f059abe8cf2, "Generalize the CMS free
>>>>> list
>>>>> code" which introduced some templates.
>>>>> It appears that GCC 4.7 is more strict when doing argument
>>>>> dependent
>>>>> lookup than 4.6.
>>>>> The fix is to do just what gcc suggests and use this->* or use the
>>>>> qualified name of the function, for example
>>>>> TreeList<Chunk>::link_tail
>>>>> in the first failure. As you can see there are already some places
>>>>> in
>>>>> that code which use the fully qualified name.
>>>>>
>>>>> I just discovered that I had a patch that fixes most of these
>>>>> issues
>>>>> in
>>>>> binaryTreeDictionary so I'll go ahead and file a CR for this.
>>>>>
>>>>> Regards
>>>>> /mg
>>>>>
>>>>
>>>> Thanks Mikael.  If you could post the patch, it would be much
>>>> appreciated.
>>>> I can't build OpenJDK8 at present.
>>>
>>> I've uploaded my fix (only build tested) to
>>> http://cr.openjdk.java.net/~mgerdin/7172226/webrev.0/
>>>
>>> /mg
>>>
>>>>
>>>>> -- 
>>>>> Mikael Gerdin
>>>>> Java SE VM SQE Stockholm
>>>>>
>>>>
>>>
>>> -- 
>>> Mikael Gerdin
>>> Java SE VM SQE Stockholm
>>>
>>
>> Thanks.  This patch worked and I've committed it to the IcedTea 
>> OpenJDK8 tree:
>>
>> http://hg.openjdk.java.net/icedtea/jdk8/hotspot/rev/f1947ffdccd9
>>
>> Can someone with JPRT access please push this through?
>