Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor Windows build fixes #528

Closed

Conversation

@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 6, 2021

Some minor changes to fix build errors/warnings on Windows:

  • Remove spurious vprintf declaration in libStdLibTest.h (it is already included as part of stdio.h). This was causing an error due to a conflicting declaration.
  • Remove return before free in benchmark lib. This was causing a warning that was then treated as an error.

Thanks,
Jorn


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/panama-foreign pull/528/head:pull/528
$ git checkout pull/528

Update a local copy of the PR:
$ git checkout pull/528
$ git pull https://git.openjdk.java.net/panama-foreign pull/528/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 528

View PR using the GUI difftool:
$ git pr show -t 528

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/panama-foreign/pull/528.diff

- Remove return before free in benchmarks
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 6, 2021

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into foreign-memaccess+abi will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label May 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 6, 2021

Webrevs

@@ -40,5 +40,4 @@ EXPORT int libc_puts(const char *str);
EXPORT struct tm *libc_gmtime(const time_t *timer);
EXPORT void libc_qsort(void *base, size_t nitems, size_t size, int (*compar)(const void *, const void*));
EXPORT int libc_rand(void);
EXPORT int vprintf(const char *format, va_list arg);
Copy link
Collaborator

@mcimadamore mcimadamore May 6, 2021

Choose a reason for hiding this comment

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

Something odd here - I note that the header has vprintf - but the impl has libc_vprintf - should we maybe add libc_vprintf in the header instead of just remove (and than make sure right symbol is used in test) ?

Copy link
Collaborator

@mcimadamore mcimadamore May 6, 2021

Choose a reason for hiding this comment

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

Btw - do we really need an header for the test library?

Copy link
Member Author

@JornVernee JornVernee May 6, 2021

Choose a reason for hiding this comment

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

Oh, ok. I misread and thought vprintf was only used to forward arguments from libc_printf, and didn't see libc_vprintf is also defined.

You're right that a header file is not really needed. I'll just remove it, and move the relevant stuff to the .c file instead.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented May 6, 2021

@JornVernee This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

Minor Windows build fixes

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the foreign-memaccess+abi branch:

  • d0a7098: 8266627: CLinker allocateMemory, freeMemory implementation should not use default lookup

Please see this link for an up-to-date comparison between the source branch of this pull request and the foreign-memaccess+abi branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the foreign-memaccess+abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 6, 2021
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented May 7, 2021

/integrate

@openjdk openjdk bot closed this May 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@JornVernee Since your change was applied there has been 1 commit pushed to the foreign-memaccess+abi branch:

  • d0a7098: 8266627: CLinker allocateMemory, freeMemory implementation should not use default lookup

Your commit was automatically rebased without conflicts.

Pushed as commit 9472dff.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@JornVernee JornVernee deleted the Win_Test_Fixes branch Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants