RFR: 7903779: unify logger messages to show position whenever possible
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print. ------------- Commit messages: - 7903779: unify logger messages to show position whenever possible Changes: https://git.openjdk.org/jextract/pull/254/files Webrev: https://webrevs.openjdk.org/?repo=jextract&pr=254&range=00 Issue: https://bugs.openjdk.org/browse/CODETOOLS-7903779 Stats: 95 lines in 10 files changed: 65 ins; 3 del; 27 mod Patch: https://git.openjdk.org/jextract/pull/254.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/254/head:pull/254 PR: https://git.openjdk.org/jextract/pull/254
On Thu, 25 Jul 2024 12:28:51 GMT, Athijegannathan Sundararajan <sundar@openjdk.org> wrote:
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print.
Marked as reviewed by jvernee (Committer). ------------- PR Review: https://git.openjdk.org/jextract/pull/254#pullrequestreview-2199195426
On Thu, 25 Jul 2024 12:28:51 GMT, Athijegannathan Sundararajan <sundar@openjdk.org> wrote:
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print.
src/main/java/org/openjdk/jextract/impl/Parser.java line 115:
113: } 114: 115: record PositionRecord(Path path, int line, int col) implements Position {}
If you want, you could make this record local to the `asPosition` method I think ------------- PR Review Comment: https://git.openjdk.org/jextract/pull/254#discussion_r1691386493
On Thu, 25 Jul 2024 12:28:51 GMT, Athijegannathan Sundararajan <sundar@openjdk.org> wrote:
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print.
Looks very good - very nice improvement! 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. 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? ------------- Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jextract/pull/254#pullrequestreview-2199256497 PR Review Comment: https://git.openjdk.org/jextract/pull/254#discussion_r1691415442 PR Review Comment: https://git.openjdk.org/jextract/pull/254#discussion_r1691413869
On Thu, 25 Jul 2024 13:09:08 GMT, Maurizio Cimadamore <mcimadamore@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
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print.
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) ------------- Changes: - all: https://git.openjdk.org/jextract/pull/254/files - new: https://git.openjdk.org/jextract/pull/254/files/3fe7c65e..a062d16b Webrevs: - full: https://webrevs.openjdk.org/?repo=jextract&pr=254&range=01 - incr: https://webrevs.openjdk.org/?repo=jextract&pr=254&range=00-01 Stats: 4 lines in 1 file changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jextract/pull/254.diff Fetch: git fetch https://git.openjdk.org/jextract.git pull/254/head:pull/254 PR: https://git.openjdk.org/jextract/pull/254
On Thu, 25 Jul 2024 12:28:51 GMT, Athijegannathan Sundararajan <sundar@openjdk.org> wrote:
* logger.warn/.err/.info Position accepting overloads. * Error message format is: <position>:"fatal"|"error"|"warning"|"info"|:<message> (consistent with clang). * ClangException has position, severity and spelling. ClangExceptions are printed using logger.print.
This pull request has now been integrated. Changeset: 7262495e Author: Athijegannathan Sundararajan <sundar@openjdk.org> URL: https://git.openjdk.org/jextract/commit/7262495ea44e384ac71d1fb71d5f0be9bc05... Stats: 95 lines in 10 files changed: 65 ins; 3 del; 27 mod 7903779: unify logger messages to show position whenever possible Reviewed-by: jvernee, mcimadamore ------------- PR: https://git.openjdk.org/jextract/pull/254
participants (3)
-
Athijegannathan Sundararajan
-
Jorn Vernee
-
Maurizio Cimadamore