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

Declare platform-independent byteswapping functions QINLINE #869

Merged
merged 1 commit into from Sep 25, 2016

Conversation

smcv
Copy link
Contributor

@smcv smcv commented Sep 25, 2016

In addition to enabling inlining for these very simple functions,
this suppresses warnings from gcc 6:

…/shared/qcommon/q_platform.h:302:13: warning: 'void CopyShortSwap(void*, const void*)' defined but not used [-Wunused-function]

The travis-ci tests on my branch demonstrate this building successfully on g++ and clang, but it would be good if someone could try this on MSVC before merging (hopefully the Appveyor builds do that for us automatically).

static QINLINE provokes warnings from clang (because QINLINE already contains static for clang), but that isn't a regression: there's a lot of that already. As a follow-up change, it would be good if the OpenJK maintainers could choose one of these options:

  • define QINLINE to inline or __inline (whichever is supported) on all compilers, the same as ID_INLINE in ioquake3; make sure that every C-style function that is declared QINLINE is static QINLINE (this resembles what is done in ioquake3)
  • define inline to __inline if the compiler doesn't support the inline keyword, and replace all QINLINE or static QINLINE with static inline (this resembles what is commonly done in Unix-centric open source projects like GLib)
  • define QINLINE to static inline or static __inline (whichever is supported) on all compilers; make sure that every C-style function that is declared QINLINE is not static QINLINE

My vote would be the first one, since OpenJK seems to be 90% of the way to it already.

In addition to enabling inlining for these very simple functions,
this suppresses warnings from gcc 6:

…/shared/qcommon/q_platform.h:302:13: warning: 'void CopyShortSwap(void*, const void*)' defined but not used [-Wunused-function]
Copy link
Member

@xycaleth xycaleth left a comment

Choose a reason for hiding this comment

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

Looks good. Would you mind submitting another PR to implement your first option? It think that sounds like the best way to go here.

@xycaleth xycaleth merged commit 96db877 into JACoders:master Sep 25, 2016
@smcv smcv deleted the byteswap-qinline branch September 25, 2016 17:48
@smcv
Copy link
Contributor Author

smcv commented Sep 25, 2016

Would you mind submitting another PR to implement your first option?

Sure, will try to get to that soon.

eternalcodes pushed a commit to eternalcodes/EternalJK that referenced this pull request Aug 21, 2018
Declare platform-independent byteswapping functions QINLINE

Former-commit-id: 82f1927
eternalcodes pushed a commit to eternalcodes/EternalJK that referenced this pull request Aug 21, 2018
Declare platform-independent byteswapping functions QINLINE
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.

None yet

2 participants