RFR: 7903779: unify logger messages to show position whenever possible [v2]

[Athijegannathan Sundararajan]

On Thu, 25 Jul 2024 13:09:08 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Athijegannathan Sundararajan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   moved PositionRecord to the method where it is used (review comment)
>
> src/main/java/org/openjdk/jextract/impl/Logger.java line 55:
> 
>> 53: 
>> 54:     public void print(ClangException exp) {
>> 55:         errWriter.println(String.format("%1$s: %2$s: %3$s",
> 
> In all these cases the "numbered" format argument is probably not necessary, as one can easily see which is which.

You're right. It was number earlier even when it had single arg in the same file. Followed it for consistency.

> src/main/java/org/openjdk/jextract/impl/Logger.java line 59:
> 
>> 57:             exp.isFatal()? "fatal" : "error",
>> 58:             exp.spelling()));
>> 59:         nErrors++;
> 
> shouldn't we increase nErrors only based on the severity of the clang diagnostic - e.g. if the diagnostic wrapped by `ClangException` is a warning, then we shouldn't increase `nErrors`. That said, I seem to recollect that the logic in `Parser` only issues a `ClangException` if the severity is > warning. I wonder it might make sense (maybe in a separate PR) to investigate whether adding support for clang warnings might make sense too?

Yes, Parser throws ClangException only for > warning level. We don't want to throw exception for less than or equal to warning level. Perhaps future change to print anything warning or less in situ?

-------------

PR Review Comment: https://git.openjdk.org/jextract/pull/254#discussion_r1691448577
PR Review Comment: https://git.openjdk.org/jextract/pull/254#discussion_r1691447642