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.

Sign up for GitHub

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

Add CMake build support #19

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Add CMake build support #19

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5e0e250
Make newstate a public header.
FtZPetruska May 27, 2023
0b6e3e9
Add common CMake build directories to gitignore.
FtZPetruska May 27, 2023
327d339
Add CMake build script.
FtZPetruska May 27, 2023
f17fbc3
Replace defines with a generated config.h.
FtZPetruska May 28, 2023
680dfad
CMake: Add support for tests.
FtZPetruska May 28, 2023
01bd891
Update build docs to use CMake.
FtZPetruska May 28, 2023
278edca
CI: Update workflow to use CMake.
FtZPetruska May 28, 2023
53b10f7
Add vcpkg config for compatibility with VS.
FtZPetruska May 28, 2023
d3a7e11
Update documentation to cover Visual Studio.
FtZPetruska May 28, 2023
4ea626a
Properly set symbols' visibility.
FtZPetruska Jun 3, 2023
ac92f8e
Add the libgambatte::libgambatte alias.
FtZPetruska Jun 3, 2023
93d5f8f
Test building with pkg-config more thoroughly.
FtZPetruska Jun 3, 2023
98e01d2
Move misplaced GBEXPORT.
FtZPetruska Jun 4, 2023
1f54590
Also include cinterface.cpp in static builds.
FtZPetruska Jun 4, 2023
93b78c0
Partially revert 4ea626ad66ba8134aef373587ce0ffe5625663a1
FtZPetruska Jun 4, 2023
78791e7
Expose the cinterface functions in gambatte-c.h
FtZPetruska Jun 4, 2023
a1f208b
Make gambatte-c usable in C.
FtZPetruska Jun 4, 2023
d1ae9ec
Use dllexport/import properly.
FtZPetruska Jun 4, 2023
fc605a8
Move FPtrs to a C-compatible header.
FtZPetruska Jun 4, 2023
472e68a
Only define dllexport/import with the shared lib.
FtZPetruska Jun 4, 2023
c75fe83
Add a simple test for the C API.
FtZPetruska Jun 4, 2023
0c8ffed
Typedef FPtrs for when it's used from C.
FtZPetruska Jun 4, 2023
79d9841
Define callback functions with C linkage.
FtZPetruska Jun 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ if(LIBGAMBATTE_BUILD_SHARED)
add_library(libgambatte-shared SHARED ${LIBGAMBATTE_SOURCES})
add_library(libgambatte::libgambatte-shared ALIAS libgambatte-shared)
list(APPEND LIBGAMBATTE_TARGETS libgambatte-shared)
target_compile_definitions(libgambatte-shared PUBLIC "LIBGAMBATTE_SHARED")
endif()

if(LIBGAMBATTE_BUILD_STATIC)
Expand Down Expand Up @@ -315,6 +316,12 @@ else()
set(LIBGAMBATTE_PC_LIBDIR "${CMAKE_INSTALL_LIBDIR}")
endif()

set(LIBGAMBATTE_PC_CFLAGS)

if(LIBGAMBATTE_BUILD_SHARED)
set(LIBGAMBATTE_PC_CFLAGS "-DLIBGAMBATTE_SHARED")
endif()

configure_file(
"${CMAKE_CURRENT_LIST_DIR}/cmake/libgambatte.pc.in"
"${libgambatte_BINARY_DIR}/libgambatte.pc"
Expand Down
2 changes: 1 addition & 1 deletion cmake/libgambatte.pc.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ includedir=${prefix}/@LIBGAMBATTE_PC_INCLUDEDIR@
Name: libgambatte
Description: The gambatte emulator core library
Version: @LIBGAMBATTE_REVISION@
Cflags: -I${includedir}/gambatte
Cflags: -I${includedir}/gambatte @LIBGAMBATTE_PC_CFLAGS@
Libs: -L${libdir} -lgambatte
Requires: @LIBGAMBATTE_PC_REQUIRES@
Requires.private: @LIBGAMBATTE_PC_REQUIRES_PRIVATE@
21 changes: 21 additions & 0 deletions libgambatte/include/fptrs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#ifndef FPTRS_H
#define FPTRS_H

#include <stddef.h> /* size_t */

#ifdef __cplusplus
namespace gambatte {
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extern "C" here is a bit interesting, technically this is correct, although in practice it typically doesn't actually matter, as most (if not all practical) C++ implementations wouldn't have any issues with a C++ linkage function being assigned to a C linkage function pointer and vice versa (as they function the same at runtime), although this isn't actually guaranteed by the standard (C++ linkage could entail a different calling convention compared to the same function being assigned C linkage, in practice this never occurs).

On that, technically this is not correctly handled in gambatte.h as they all would use C++ (default) linkage, so if we want that to be ""correctly"" done, all those callback definitions need extern "C".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the off chance that there is a compiler out there that defaults to using a different calling convention between C and C++, I went ahead and declared the other function prototypes as extern "C" for consistency.

Moreover, to make sure the C API is actually usable from C, I added a simple test in c75fe83 that creates and deletes a GB object using the C API.

#endif
struct FPtrs {
void (*Save_)(void const *ptr, size_t size, char const *name);
void (*Load_)(void *ptr, size_t size, char const *name);
void (*EnterSection_)(char const *name);
void (*ExitSection_)(char const *name);
};
#ifdef __cplusplus
}
}
#endif

#endif /* FPTRS_H */
8 changes: 7 additions & 1 deletion libgambatte/include/gambatte-c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@
#include <stdint.h>
#include <stdbool.h>

#include "fptrs.h"

typedef struct GB GB;
typedef struct FPtrs FPtrs;
typedef unsigned(InputGetter)(void *);
typedef void (*MemoryCallback)(int32_t address, int64_t cycleOffset);
typedef void (*CDCallback)(int32_t addr, int32_t addrtype, int32_t flags);
#else
#include "fptrs.h"
#include "gambatte.h"
#include "newstate.h"

Expand All @@ -43,6 +45,7 @@ using MemoryCallback = gambatte::MemoryCallback;
using CDCallback = gambatte::CDCallback;
#endif

#if defined(LIBGAMBATTE_SHARED)
#if defined(_WIN32) || defined(__CYGWIN__)
#if defined(LIBGAMBATTE_DLL_EXPORT)
#define GBEXPORT __declspec(dllexport)
Copy link
Member

@CasualPokePlayer CasualPokePlayer Jun 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is used by the user of a dll rather than the dll itself, then dllexport would be incorrect, dllimport is the correct declspec (and I don't think visibility attribrute is correct either for non-Windows cases, rather it should just be extern)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking GCC's documentation on visibility, you're correct about the dllexport/dllimport.

According to that same documentation, for the non-Windows case, __attribute__((visibility("default"))) seems to indicate both export and import.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the change in d1ae9ec broke CI.

Looking around, other projects such as SDL seem to only be using dllexport.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking CI likely indicates it got used internally in the library, or it got used when linking it in statically. You can only use dllimport when you are a user of the library (so not the library itself) and the library is a dll (if it's static, you don't use dllimport/dllexport).

Expand All @@ -54,6 +57,9 @@ using CDCallback = gambatte::CDCallback;
#else
#define GBEXPORT
#endif
#else /* !defined(LIBGAMBATTE_SHARED) */
#define GBEXPORT
#endif

#ifdef __cplusplus
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this might be expected to be #included from a C context (not C++), it may be better to #ifdef this around __cplusplus.

Similarly, instead of #including gambatte.h or newstate.h, it may be better to just forward declare struct GB instead, (and other redeclarations from the gambatte namespace as that might not be available in a C context), at the very least do that when __cplusplus is not defined.

Expand Down
8 changes: 1 addition & 7 deletions libgambatte/include/newstate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <cstring>
#include <cstddef>

#include "fptrs.h"
namespace gambatte {

class NewState {
Expand Down Expand Up @@ -41,13 +42,6 @@ class NewStateExternalBuffer : public NewState {
const long maxlength;
};

struct FPtrs {
void (*Save_)(void const *ptr, std::size_t size, char const *name);
void (*Load_)(void *ptr, std::size_t size, char const *name);
void (*EnterSection_)(char const *name);
void (*ExitSection_)(char const *name);
};

class NewStateExternalFunctions : public NewState {
public:
NewStateExternalFunctions(const FPtrs *ff);
Expand Down