8313396: Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION #17387
Conversation
|
👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into |
openjdk
bot
changed the title
|
@TheShermanTanker The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
…compilerWarnings.hpp
|
@kimbarrett This isn't ready for review yet, but I think I might need your help, how was your original approach able to avoid firing warnings and errors when use of forbidden methods was encountered in third party code, such as GTest? Looking at the logs mine is failing because the forbidden methods are being detected in GTest code as invalid |
|
Opening to review, because I really need help with this |
|
❗ This change is not yet ready to be integrated. |
|
@TheShermanTanker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@TheShermanTanker this pull request can not be integrated into git checkout patch-12
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
|
@TheShermanTanker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@TheShermanTanker This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
/open |
|
@TheShermanTanker This pull request is now open |
|
@TheShermanTanker This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Please review a portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION:
Currently, FORBID_C_FUNCTION only works for gcc like compilers, and ALLOW_C_FUNCTION acts to disable CRT warnings on Windows, where FORBID_C_FUNCTION does not work. It would be beneficial to provide a universal portable definition for both, to allow the macros to work on all platforms HotSpot can be compiled for.
The implementation is portable and should work on all HotSpot supported platforms (I don't have an AIX device!).
Regrettably, I did end up having to change the signature of ALLOW_C_FUNCTION to work with this new implementation, as well as the way it is used. On one hand, it is more compact than before, but on the other the established syntax is likely more familiar by this point. I do hope this is not a showstopper, but understand if it is
Progress
Error
Warnings
Issue
Backport <hash>with the hash of the original commit. See Backports.Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17387/head:pull/17387$ git checkout pull/17387Update a local copy of the PR:
$ git checkout pull/17387$ git pull https://git.openjdk.org/jdk.git pull/17387/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17387View PR using the GUI difftool:
$ git pr show -t 17387Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17387.diff
Using Webrev
Link to Webrev Comment