(Feedback requested) Break MINIMAL_CORE into separate feature flags #2068
Conversation
Changes some of the condition compilation from opting out when a flag is defined ("ifndef MINIMAL_CORE") to having the flag always be defined as a 1 or 0.
This also adds functions for checking feature flags at runtime (mGBA_flag_threading, mGBA_flag_renderer_software). Useful for when the library is used in an FFI/dynamically-loaded context.
|
Figure @kode54 may be interested in reviewing this from the GSF perspective |
There was a problem hiding this comment.
There is a LOT wrong with this PR. It looks like you didn't even attempt to figure out the style since it's pretty consistent and you just...ignored it wholesale, and you didn't read CONTRIBUTING.md either.
Further, don't use 0/1 defines, since those don't match existing style either.
I appreciate that you want to contribute but you should try to adhere to existing guidelines if you want to contribute to, well, any project.
| set(MGBA_ENABLE_VIDEO_LOGGER ON CACHE BOOL "Enable video logger") | ||
| set(MGBA_ENABLE_FILESYSTEM ON CACHE BOOL "Enable filesystem support") | ||
| set(MGBA_ENABLE_INPUTMAP ON CACHE BOOL "Enable input mapping interface") | ||
| set(MGBA_ENABLE_THREADING ON CACHE BOOL "Enable threading") | ||
| set(MGBA_ENABLE_RENDERER_SOFTWARE ON CACHE BOOL "Enable software renderer") |
There was a problem hiding this comment.
These should absolutely not be cached since many of them will be unique to each output lib. Further, since they aren't cached, they shouldn't be namespaced either.
| @@ -776,6 +787,7 @@ add_subdirectory(src/util) | |||
|
|
|||
| list(APPEND GUI_SRC ${EXTRA_GUI_SRC}) | |||
| list(APPEND UTIL_SRC ${CMAKE_CURRENT_BINARY_DIR}/version.c) | |||
| list(APPEND CORE_SRC ${CMAKE_CURRENT_BINARY_DIR}/flags.c) | |||
There was a problem hiding this comment.
If version.c is in UTIL_SRC why is flags.c in CORE_SRC? (They should both be in UTIL_SRC since up to this point CORE_SRC is only for the emulation bits, not the periphery bits)
| extern "C" { | ||
| #endif | ||
|
|
||
| int |
There was a problem hiding this comment.
This style is totally wrong. Please take a look at existing files to get an actual idea, or read the (woefully outdated) CONTRIBUTING.md file. Naming scheme should likely be mFlagGet, and an enum of flags instead of individual functions would probably be better. Further, MGBA_EXPORT should be used.
| @@ -119,4 +134,25 @@ | |||
| #cmakedefine USE_ZLIB | |||
| #endif | |||
|
|
|||
| /* functions to query flags at runtime instead of compile time */ | |||
| #ifdef __cplusplus | |||
| extern "C" { | |||
There was a problem hiding this comment.
This goes around the whole file if needed. You should also include mgba-util/dllexports.h
| @@ -3,6 +3,7 @@ | |||
| * This Source Code Form is subject to the terms of the Mozilla Public | |||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
| #include <mgba/flags.h> | |||
There was a problem hiding this comment.
Includes should be alphabetical order.
|
|
||
| if(MGBA_ENABLE_VIDEO_LOGGER) | ||
| list(APPEND SOURCE_FILES video-logger.c) | ||
| endif(MGBA_ENABLE_VIDEO_LOGGER) |
There was a problem hiding this comment.
| endif(MGBA_ENABLE_VIDEO_LOGGER) | |
| endif() |
| @@ -5,6 +5,7 @@ | |||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
| #include <mgba/internal/gba/extra/cli.h> | |||
|
|
|||
| #include <mgba/flags.h> | |||
| @@ -3,12 +3,15 @@ | |||
| * This Source Code Form is subject to the terms of the Mozilla Public | |||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | |||
| #include <mgba/flags.h> | |||
| #include <mgba/core/config.h> | ||
| #include <mgba/core/core.h> | ||
| #include <mgba/core/log.h> | ||
| #include <mgba/core/version.h> | ||
| #include <mgba/feature/commandline.h> | ||
| #if MGBA_ENABLE_VIDEO_LOGGER |
There was a problem hiding this comment.
This will not work without video logging. Just disable this frontend.
Changes some of the condition compilation from opting out when a flag is defined (
#ifndef MINIMAL_CORE) to having the flag always be defined as a 1 or 0.This also adds functions for checking feature flags at runtime (
mGBA_flag_threading,mGBA_flag_renderer_software). Useful for when the library is used in an FFI/dynamically-loaded context. These functions useCMAKE_PROJECT_NAME, so once medusa is merged in they will be automatically changed.This did require bringing flags.h in internally (I think before, the only code that included it was a python binding).
I envisioned the other flags could become similar (
BUILD_GL=>MGBA_ENABLE_RENDERER_GLandmGBA_flag_renderer_gl(),BUILD_GLES2=>MGBA_ENABLE_RENDERER_GLES2andmGBA_flag_renderer_gles2(), etc).I prefixed these defines with
MGBA_, I figure it won't be hard to do a find/replace with a new name once medusa is merged in. This way, when a downstream user includesflags.h, all imported defines have a prefix, to prevent conflicts with their own defines.I tried to bring in changes from #1288 as well.
Open to any/all feedback/criticism/suggestions, especially re: naming conventions, style, etc. I wasn't entirely sure what the "correct" name for some of these features are.