RFR(M): Graal PTX enhancements
Thomas Wuerthinger
thomas.wuerthinger at oracle.com
Tue Apr 9 08:36:40 PDT 2013
Gilles,
Could you please add automatic sanity checks regarding System.err.println and System.out.println to our check style configuration (see http://checkstyle.sourceforge.net/config_regexp.html)?
Thanks, thomas
On Apr 9, 2013, at 5:24 PM, Gilles Duboscq <duboscq at ssw.jku.at> wrote:
> It's not so important but there are still some System.err.println (in
> PTXLIRGenerator.java, PTXCompare.java and PTXControlFlow.java)
>
>
> On Tue, Apr 9, 2013 at 5:03 PM, Morris Meyer <morris.meyer at oracle.com>wrote:
>
>> Thanks Gilles, Doug and Christian for the review. Here is my updated
>> webrev addressing those concerns.
>>
>> --mm
>>
>> WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.03
>>
>>
>> On 4/9/13 7:59 AM, Gilles Duboscq wrote:
>>
>> There are some System.(out|err).println in various files could you remove
>> them? When there are things like throw GraalInternalError.shouldNotReachHere(),
>> if you need some output you can also use GraalInternalError.shouldNotReachHere(String
>> message) instead of the println
>>
>> Otherwise it looks good
>>
>>
>> On Tue, Apr 9, 2013 at 1:59 PM, Gilles Duboscq <gilwooden at gmail.com>wrote:
>>
>>> There are some System.(out|err).println in various files could you remove
>>> them? When there are things like
>>> throw GraalInternalError.shouldNotReachHere(), if you need some output you
>>> can also use GraalInternalError.shouldNotReachHere(String message) instead
>>> of the println
>>>
>>> Otherwise it looks good
>>>
>>>
>>> On Sat, Apr 6, 2013 at 1:18 AM, Morris Meyer <morris.meyer at oracle.com>wrote:
>>>
>>>> Folks
>>>>
>>>> There is an updated webrev with Christian's review changes here:
>>>>
>>>> WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.02
>>>>
>>>> --morris
>>>>
>>>>
>>>> On 4/5/13 6:09 PM, Morris Meyer wrote:
>>>>
>>>>> Thanks for the review.
>>>>>
>>>>> Actually with JavaFX, explicit class declarations were part of our
>>>>> check-in procedures. I put the changes into PTXLIRGenerator as IMHO it
>>>>> helps folks just looking at the source to see what makes up the class.
>>>>>
>>>>> Will change the class to PTXTestBase and look after the indenting.
>>>>>
>>>>> --mm
>>>>>
>>>>>
>>>>> On Apr 5, 2013, at 5:42 PM, Christian Thalinger <
>>>>> christian.thalinger at oracle.com> wrote:
>>>>>
>>>>> On Apr 5, 2013, at 10:46 AM, Morris Meyer <morris.meyer at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> Folks,
>>>>>>>
>>>>>>> Could I get a review of my recent PTX enhancements to Graal? I've
>>>>>>> changed the tests to have a base class, extended the assembler, and have
>>>>>>> taken the test coverage of PTX from 2 to 23 tests.
>>>>>>>
>>>>>>> Thanks in advance,
>>>>>>>
>>>>>>> --morris meyer
>>>>>>>
>>>>>>> WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.01
>>>>>>>
>>>>>> This is great!
>>>>>>
>>>>>> Looking at:
>>>>>>
>>>>>>
>>>>>> graal/com.oracle.graal.compiler.ptx/src/com/oracle/graal/compiler/ptx/PTXLIRGenerator.java
>>>>>>
>>>>>> it seems we need to turn off "Organize imports" in our favorite
>>>>>> editor. Also indenting seems to be an issue.
>>>>>>
>>>>>> I know the answer is "use the mx script to create the IDE
>>>>>> configuration" but no everybody is using Eclipse or Netbeans. We need to
>>>>>> find a way to keep this consistent without the help of an IDE (maybe
>>>>>> Mercurial push hooks?).
>>>>>>
>>>>>>
>>>>>> graal/com.oracle.graal.compiler.ptx.test/src/com/oracle/graal/compiler/ptx/test/PTXCompilerBase.java:
>>>>>>
>>>>>> Can we name the test base class PTXTestBase?
>>>>>>
>>>>>> Otherwise this looks good to me.
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>
>>>>
>>>
>>
>>
More information about the graal-dev
mailing list