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

replace sOffset (use offsetof) #4632

Merged
merged 1 commit into from
Nov 26, 2023
Merged

replace sOffset (use offsetof) #4632

merged 1 commit into from
Nov 26, 2023

Conversation

jg1uaa
Copy link
Contributor

@jg1uaa jg1uaa commented Nov 26, 2023

Building DOSBox-X on OpenBSD/amd64 (clang-13), I see many -Wnull-pointer-subtracion warnings like this:

../../include/dos_inc.h:542:35: warning: performing pointer subtraction with a null pointer may have undefined behavior [-Wnull-pointer-subtraction]
        void SetBootDrive(uint8_t drv) { sSave(sDIB,bootDrive,drv); }
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~
../../include/dos_inc.h:395:61: note: expanded from macro 'sSave'
#define sSave(s,m,val) SaveIt(sizeof(((s *)&pt)->m),(PhysPt)sOffset(s,m),val)
                                                            ^~~~~~~~~~~~
../../include/dos_inc.h:393:46: note: expanded from macro 'sOffset'
#define sOffset(s,m) ((char*)&(((s*)NULL)->m)-(char*)NULL)
                                             ^~~~~~~~~~~~

I think sOffset() in dos_inc.h can be replaced with offsetof() and it can reduce this warning. Is there any historical reason to keep this code?

(in OpenBSD's stddef.h, offsetof() is defined like this)

#if __GNUC_PREREQ__(4, 0)
#define offsetof(type, member)  __builtin_offsetof(type, member)
#else
#define offsetof(type, member)  ((size_t)(&((type *)0)->member))
#endif

@jg1uaa jg1uaa changed the title replace sOffset -> offsetof replace sOffset (use offsetof) Nov 26, 2023
@maron2000
Copy link
Contributor

maron2000 commented Nov 26, 2023

I didn't try but one of the concern is that whether the code is C++11 compatible.
The following is sited from the contribution guide.

DOSBox-X is still intended to support compilation as C++11.
 If you add new code please make sure it works as C++11. 
One way to do so is to run ./configure with --enable-force-cxx11 option added, 
and build the code.

@jg1uaa
Copy link
Contributor Author

jg1uaa commented Nov 26, 2023

I checked this change does not make error to compile with gcc-8.4.0 and clang-13 on OpenBSD,
with or without configure --enable-force-cxx11.

at least, dos_inc.h has already included <stddef.h> to use offsetof().
https://github.com/joncampbell123/dosbox-x/blob/master/include/dos_inc.h#L28

@joncampbell123
Copy link
Owner

joncampbell123 commented Nov 26, 2023

offsetof() is way older than C++11, it shouldn't be a problem.

https://en.cppreference.com/w/cpp/types/offsetof

It was there because the original DOSBox project had it too.

Perhaps DOSBox Staging might appreciate this change as well, since they also intend to modernize their code base (and sometimes even sneer at me a little for not jumping onto the C++17 bandwagon).

@joncampbell123 joncampbell123 merged commit a6047da into joncampbell123:master Nov 26, 2023
17 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.

3 participants