Rename lmms_basics.h and scope it down to type declaration
#7677
Conversation
|
I am still not sure about copy pasting the macro directly to the other places but the "modern" methods I looked at to implement this functionality turned out to be very janky compared to this simple macro. |
Rossmaxx
changed the title
lmms_basics.h and simplify the LMMS_STRINGIFY macro to be used at call sites.lmms_basics.h and scope it down to type declaration
This reverts commit a55fcdf.
|
Do note that the diff itself is small. Most of the thing is due to the file rename in the #include statement |
|
Now i should hunt down any unused includes over here, but I don't really have the insanity to do that manually. |
|
I'll fix the declaration later. |
This reverts commit a832ded.
|
Seems like I got myself into a rabbit hole with this refactoring |
CMakeLists.txt
Outdated
| @@ -630,6 +630,10 @@ IF(NOT CMAKE_BUILD_TYPE) | |||
| "MinSizeRel" "RelWithDebInfo") | |||
| ENDIF() | |||
|
|
|||
| IF(CMAKE_BUILD_TYPE STREQUAL "Release") | |||
| add_compile_definitions(NDEBUG) | |||
There was a problem hiding this comment.
This seems incorrectly placed.
- Prior to this PR, setting
-DLMMS_DEBUGwould UNSETNDEBUG, now you're doing this based onCMAKE_BUILD_TYPEwhich modifies the behavior to something else. - I believe what you meant to do here was to only set
NDEBUGwhenLMMS_DEBUGwas NOT set. If that's the case, the code should be updated to reflect that. Furthermore we should also document this in our debug flags, e.g.
Developer options
-----------------------------------------
* Debug FP exceptions : Disabled
* Debug using AddressSanitizer : Disabled
* Debug using ThreadSanitizer : Disabled
* Debug using MemorySanitizer : Disabled
* Debug using UBSanitizer : Disabled
* Debug packaging commands : Disabled
* Profile using GNU profiler : Disabled
+ * Debug assertions : Disabled
There was a problem hiding this comment.
I would just call it "Debug assertions" rather than "Treat assertions as fatal" because failed assertions are always fatal.
There was a problem hiding this comment.
Update, it seems cmake already handles this, so should i even bother?
There was a problem hiding this comment.
Update, it seems cmake already handles this, so should i even bother?
I don't believe this statement to be true. Please be more specific, as my reply is specific and contains instructions to resolve this problem.
lmms_basics.htoLmmsTypeDeclarations.hand scoped it down for that purpose.LMMS_STRINGIFYmacro's declaration to call sites.