Skip to content

Conversation

@kcgen
Copy link

@kcgen kcgen commented Apr 16, 2021

This PR:

  1. Fixes the following Effective C++ warnings under Clang and GCC by making initial member values explicit:

    2021-04-15_20-50

  2. Prevents the compiler from automatically creating default copy and assignment constructors for the SID class, as we don't want it copied or assigned given it contains pointers and dynamic memory (which the compiler doesn't handle in its generated defaults).

@simonowen
Copy link
Owner

The warning fixes look good but the class rename is an interface breaking change for existing users of the library. It might be better if the default behaviour was backwards compatible, but with a way for newer projects to opt-in to the rename?

I've been stung by the same symbol conflict, and used a pre-processor work-around when building under Windows. I use #define SID WIN32_SID before including any Windows headers, then a #undef SID before including resid.h. Though that might not be suitable for larger projects like DOSBox, or projects that use the real Win32 SID symbol.

An alternative I was considering was to wrap the original project in a reSID namespace, and if a pre-processor symbol is not defined when resid.h is included it'll add a using namespace reSID to pull everything into top-level scope like before. Newer projects can then just define a symbol to use resid::SID without the unwanted symbol pollution.

Any thoughts?

@kcgen
Copy link
Author

kcgen commented Apr 17, 2021

Thanks for the quick review @simonowen.

From what I've seen, every project using reSID has copied it into their project - likewise I haven't see reSID distributed as a library - so the concern of projects passively breaking (through some routine OS upgrade) is very unlikely (unlike, say if the zlib author broke their API).

So given this, I would opt to avoid preprocessor hacks and go for the namespace solution, as that's a perfectly acceptable and reasonable API solution in C++. Infact, I would even avoid that last bit regarding defining a symbol. If a project really wants to avoid the namespace, then it's pretty easy to add using namespace reSID and they're done :-)

Given neither option involve renaming the class, I will drop my second commit!

@kcgen kcgen changed the title Fix -Weffc++ warnings and avoid a name-conflict under Windows Fix -Weffc++ warnings Apr 17, 2021
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