[foreign] RFR jextract should model clang Cursors as Trees

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Mon Sep 3 10:42:15 UTC 2018

Looks good. As a followup I'd also like to see Parser::main removed and 
turned into some jextract debug option, so that we can write tests 
against it.


On 03/09/18 11:34, Sundararajan Athijegannathan wrote:
> I've made few further changes:
> * Removed main method AsmCodeFactory class
> * TreePrinter refactored as a separate class and Parser's (test) main 
> is now uses it
> * Removed CallbackTree (yes, it was from an earlier attempt) and 
> associated visitor method
> Updated: http://cr.openjdk.java.net/~sundar/8210263/webrev.01
> Thanks.
> -Sundar
> On 31/08/18, 10:26 PM, Maurizio Cimadamore wrote:
>> Great work; I think this goes a long way towards putting some more 
>> solid foundation under jextract. It also allows, by putting most of 
>> the clang dependencies right into the immediate frontend, to make the 
>> rest of the code less sensitive to API changes (e.g. should we go 
>> from C to C++ clang API). Some comments below (some of those might be 
>> overly general and you might want to deal with them in followup 
>> changes):
>> * Thanks for adding the equals/hashcode method in Source[Location, 
>> Range]
>> * While it's probably early now, I think there are a lot of steps 
>> which might be translated into their own visitor - e.g. filtering 
>> cursors seems one of them (happens between Context and Parser)
>> * I also believe that, moving forward, classes such as Context and 
>> HeaderFile should be all about keeping state, and not about defining 
>> behavior (which something that should be delegated to tree visitors)
>> * I like that AsmCodeFactory just become a visitor!
>> * Macro handling became a bit better (since we now compute macros on 
>> parsing, and then delegate generation to the code factory) - I like 
>> that; but let's also keep in mind that macro computation can become 
>> its own tree step
>> * Nit: why is there a main method in ASMCodeFactory?
>> * It feels like inside Parser there's a tree printer (see main 
>> method). And that this printer should probably be unified with the 
>> main Printer class. And, pulling on this string some more, I'd like 
>> this printer exposed as its own class, so that we can use it to unit 
>> test parsing.
>> * It feels like we could decouple cursors from the tree even more; 
>> for instance, names are not reified in the AST (and types, too). This 
>> means that it is not possible, for instance, to define a visitor step 
>> which replaces some of the names (e.g. something like that would be 
>> needed for typedefs). The code, as now, will keep going back to the 
>> cursor.
>> * what exactly is CallbackTree? It seems unused (and so is one of the 
>> two createEnum overloads). I believe that is a leftover of some 
>> earlier attempts?
>> P.S.
>> Fetched patch, ran all tests, all clear on Linux x64.
>> Cheers
>> Maurizio
>> On 31/08/18 16:13, Sundararajan Athijegannathan wrote:
>>> Please review.
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210263
>>> Webrev: http://cr.openjdk.java.net/~sundar/8210263/webrev.00/
>>> Thanks,
>>> -Sundar

More information about the panama-dev mailing list