[foreign] RFR jextract should model clang Cursors as Trees

[Maurizio Cimadamore]

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.

Maurizio


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
>>