-
Notifications
You must be signed in to change notification settings - Fork 7
Eliminate circular DTYPES dependency in RCDEF #413
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
Conversation
…rcdef-untangle
| @@ -1,7 +1,9 @@ | |||
| cmake_minimum_required(VERSION 3.15.0) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a necessary update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake stuff I've read recommends putting cmake-minimum_required as the first thing in the file, so I moved it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But no particular reason to upgrade to 3.15 from 3.10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the change was inadvertent. I have done some reading in Professional CMake and noted many features added in 3.15 related to debugging, clang-on-Windows-support, and other things. Given that 3.15 is now 4 years old, it is not unreasonable to require it. In fact, I would advocate going much newer and learning how to exploit current features. We're a small group, it would not be a major inconvenience to move to a newer version.
src/libstubs.cpp
Outdated
| #if 0 | ||
| #include "cnglob.h" | ||
| #include "vrpak.h" | ||
|
|
||
| VROUTINFO5 PriRep = { { 0 } }; | ||
| const char* InputFilePathNoExt = NULL; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this or not? If it is sticking around, should have some comments about why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it should be deleted, will do that.
test/unit/CMakeLists.txt
Outdated
| xiopak.unit.cpp | ||
| cvpak.unit.cpp | ||
| $<TARGET_OBJECTS:cse_libstubs> | ||
| ../../src/libstubs.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ${CSE_SOURCE_DIR} instead of ../../src.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
Description
Rework build scheme so rcdef does not use dtypes.h, eliminating circular dependency. This allows straightforward switch between 32 and 64 bit builds.
Files included in the rcdef build have been modified to compile using only a small subset of dtypes.h definitions. Those are provided in cnglob.h when NODTYPES is #defined. Some dtypes.h typedef types have been coded out in non-cnrecs.def usage (notably BOO (= short integer) has been replaced by bool).
Also combined build and test jobs in the CI workflow. This allows unit tests to use the current dtypes.h.