On Mon, 8 Aug 2022 13:25:22 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> JLI_ReportErrorMessageSys has a number of issues, as listed below: >> >> - The windows variant prints message, then extra-info if available, but the >> Unix variant prints first extra-info, then message on a newline. Standardize >> both to print their system errors on newlines below the message. >> >> - The Windows implementation intermingles GetLastError and errno, which is >> incorrect. Both can be set, from independent API calls, and without knowing >> which API was called we don't know which to display. There is no way for >> JLI_ReportErrorMessageSys to know which is the right code. As it is, in its >> current form, we may call JLI_ReportErrorMessageSys for a CRT error and it >> may accidentally display a Win32 API error lingering around from some >> earlier unrelated Win32 API call. Changing JLI_ReportErrorMessageSys to add >> Windows specific components is not desired, so as a compromise we can simply >> print both Win32 and C Runtime errors together, if both exist. >> >> - On macOS, we have two call sites of JLI_ReportErrorMessageSys where the >> errno text is already provided as part of the message by the caller, and >> therefore would be printed twice. >> >> - Visual separation of message and extra-message: the rule is apparently >> that the caller has to provide the separation formatting: "it's up to the >> calling routine to correctly format the separation of messages" (of message >> and extra info). This leads to very awkward call sites (see >> https://github.com/openjdk/jdk/pull/9749). The problem is that the extra >> info may not be available: errno or GetLastError may report 0. That cannot >> be predicted by the caller, therefore the caller should not have to think >> about it. The function should append the extra-info only if available, omit >> it if it is not available, and take care of separation itself. >> >> Side note: The javaw implementation in this patch for Windows is a bit of a >> mess, advice or improvements to it are appreciated > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > getLastErrorString can be used for Unix Hi Julien, Just a tip: as long as you are in the active development phase of a patch, you can mark the PR as "draft". That way you do not spam the mailing lists with updates for every single push. And you save valuable Reviewer cycles since they won't have to look at the half-finished review. Once you are content with the patch, and all tests are green, you can mark the PR as "ready for review". Only then Review mails are sent out. --- That said, I don't think printing both codes really works. See my comment in JBS. Cheers, Thomas ------------- PR: https://git.openjdk.org/jdk/pull/9793
