Skip to content

Conversation

@baier233
Copy link
Contributor

@baier233 baier233 commented Oct 18, 2025

See issue #1695

@matthiasblaesing
Copy link
Member

matthiasblaesing commented Oct 19, 2025

Thanks for the contribution. A few nitpicks:

  • Please check the author name and ensure that the name information is valid (it currently is a nickname) (see second line of https://patch-diff.githubusercontent.com/raw/java-native-access/jna/pull/1696.patch)
  • It would be good to keep the constructors of the class aligned, so could you please ensure that both the parameterless and the one with the long parameter work for the ByReference variant?
  • The commit message might be better "Add LARGE_INTEGER.ByValue to LARGE_INTEGER in WinNT.java"
  • Please document the change in CHANGES.md in the root of the tree as a feature

@baier233 baier233 force-pushed the master branch 2 times, most recently from fd2b781 to 98e1578 Compare October 19, 2025 09:09
@baier233
Copy link
Contributor Author

Thanks for the contribution. A few nitpicks:

  • Please check the author name and ensure that the name information is valid (it currently is a nickname) (see second line of https://patch-diff.githubusercontent.com/raw/java-native-access/jna/pull/1696.patch)
  • It would be good to keep the constructors of the class aligned, so could you please ensure that both the parameterless and the one with the long parameter work for the ByReference variant?
  • The commit message might be better "Add LARGE_INTEGER.ByValue to LARGE_INTEGER in WinNT.java"
  • Please document the change in CHANGES.md in the root of the tree

Thanks for the review and the detailed feedback! I have addressed all the nitpicks:

  1. Author Name: Corrected the author name in the commit history.
  2. Constructors: Ensured the parameterless and long parameter constructors are both available and working for the ByReference variant.
  3. Commit Message: Amended the commit message to: "Add LARGE_INTEGER.ByValue to LARGE_INTEGER in WinNT.java"
  4. Documentation: Added a feature entry to CHANGES.md.

Please let me know if there's anything else needed for review!

CHANGES.md Outdated
Comment on lines 1468 to 1472

Features
--------

* [#1696](https://github.com/java-native-access/jna/pull/1696): Add `LARGE_INTEGER.ByValue` to `LARGE_INTEGER` in `WinNT.java` - [@Baier](https://github.com/baier233).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this into the "Features" section at the top of the file. The entries are sorted anti chronological (newest first). There is always a section for the next release (5.19.0). The part inside the square brackets should be the github username.

@baier233 baier233 force-pushed the master branch 2 times, most recently from 6e39200 to fb9a5f1 Compare October 20, 2025 07:11
@matthiasblaesing
Copy link
Member

Checkstyle is not happy:

Error: tyle] [ERROR] /home/runner/work/jna/jna/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java:1197: Line has trailing spaces. [RegexpSingleline]

@baier233
Copy link
Contributor Author

Checkstyle is not happy:

Error: tyle] [ERROR] /home/runner/work/jna/jna/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java:1197: Line has trailing spaces. [RegexpSingleline]

Oops,everything should be fine now.

@matthiasblaesing
Copy link
Member

Looks sane. Thank you.

@matthiasblaesing matthiasblaesing merged commit 0deb54b into java-native-access:master Oct 20, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants