[foreign] RFR 8219967: Add clang document introspection API support in jdk.internal.clang module

[Sundararajan Athijegannathan]

Thanks for your review!

* added assert after all constructor lookup calls
* removed unnecessary inlineContentHasTrailingNewline0 indirection
* "indent" was the intent. oops :)
* added space after copyright uniformly.

Updated: https://cr.openjdk.java.net/~sundar/8219967/webrev.01/

Thanks,
-Sundar

On 01/03/19, 5:31 PM, Maurizio Cimadamore wrote:
>
> Full review:
>
> * cleanup of native code is nice - I like that you enforced foo0 
> everyehere
>
> * jdk_internal_clang.cpp - missing an assert after constructor lookup, 
> at the top
>
> * kudos for implementing the full API - I can see how tedious it was :-)
>
> * Comment.java - why this:
>
> public boolean inlineContentHasTrailingNewline() {
>    50         return inlineContentHasTrailingNewline0();
>    51     }
>
> E.g. why the 0-version if that's what we want to return in the public 
> API too?
>
> * Comment::intent, I think you meant "indent" ?
>
> * Nits: there seems to be a missing newline after copyright in all new 
> files but Comment.java (e.g. all the new enums)
>
> Very good piece of work.
>
> Maurizio
>
>
>
> On 01/03/2019 11:32, Maurizio Cimadamore wrote:
>
>>
>> On 01/03/2019 10:09, Sundararajan Athijegannathan wrote:
>>> Please review.
>>>
>>> Rfe: https://bugs.openjdk.java.net/browse/JDK-8219967
>>> Webrev: https://cr.openjdk.java.net/~sundar/8219967/webrev.00/
>>>
>>> PS. Piggybacking to make few cleanups:
>>>
>>> Made few native methods in jdk.internal.clang private.
>>> Renamed all native methods as foo0 (kind0) pattern as used elsewhere 
>>> in JDK rather than foo1.
>>>
>>> I've not added tests because other parts of jdk.internal.clang don't 
>>> seem to have direct tests - those are exercised only via 
>>> jdk.jextract tests. For now, I ran Comment class (which has a test 
>>> main) on few clang headers with documentation and manually inspected 
>>> the output.
>>
>> Quick idea for testing - if we added printing of comment in the 
>> cursor (in TreePrinter), then we could have tests which check the 
>> well-formedness of the AST (including comments)? I also have added 
>> tree evaluation support in there, so it seems like starting to write 
>> tree-printer based test could be a good idea (maybe not part of this 
>> changeset, but as a followup). Reminds me of what we have done in 
>> javac with the DPrinter framework:
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/tip/test/langtools/tools/javac/lib/DPrinter.java 
>>
>>
>> Maurizio
>>
>>>
>>> Thanks,
>>> -Sundar