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.

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

Remove implementation files, make same API for inlining and shared library #135

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

Conversation

tylov
Copy link

@tylov tylov commented Apr 11, 2020

  • Remove all include/cglm/call.h, include/cglm/call/.h and src/.c files, while keeping option to compile and use CGLM as a shared library.
  • ONLY ONE API, and switch between compiling it inline or using shared library by just defining CGLM_LIB.
  • Simplify maintenance and reduce number of files.
    Details:
  • Build library by compiling src/cglm.c. This defines CGLM_DLL
  • Use library by defining -DCGLM_LIB and link with created lib (dll or so).

FIXES/changes in include/cglm/common.h:

  • '__declspec(...)' was only enabled for Visual Studio, but all major compilers on windows uses this, including MinGW / Cygwin GCC, Tiny C, etc.
  • 'attribute((visibility("default")))' was used for both import and export for all other compilers than VC, but should generally only be enabled for export with GCC.
  • 'static inline __attribute((always_inline))' was used for all compilers other than VC, should generally be used for GCC, otherwise use 'static inline'.

Note: It should be trivial to extend common.h so that you could e.g. define CGLM_STATIC_LIB as a third compilation option in addition to inline and shared library.

Tested on windows gcc (tdm64) 9.2, Visual studio 2019, Tiny C.
Code changes were done in Notepad++ using the regular expressions below (on all open files). Only the first function in io.h and the tests needed to be edited manually.

find 1: CGLM_INLINE
repl 1: CGLM_DECL

find 2: ) {\r\n(#if)
repl 2: ) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n{\r\n$1

find 3: ) {\r\n( [^ ])
repl 3: ) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n{\r\n$1

find 4: ^}\r\n\r\n
repl 4: }\r\n#endif\r\n\r\n

- Remove all include/cglm/call.h, include/cglm/call/*.h and src/*.c files, while keeping option to compile and use CGLM as a library.
- Have ONE API ONLY, and switch between compiling it inline or using library by just defining CGLM_LIB.
- Simplify maintenance and reduce number of files.
Details:
- Build library by compiling src/cglm.c. This defines CGLM_DLL
- Use library by defining -DCGLM_LIB and link with created lib (dll or so).

Main task was done using the regular expressions in Notepad++ below. Only first function in io.h done manually.
Logic added/improved in include/cglm/common.h

find 1: CGLM_INLINE
repl 1: CGLM_DECL

find 2: \) \{\r\n(#if)
repl 2: \) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n\{\r\n$1

find 3: \) \{\r\n(  [^ ])
repl 3: \) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n\{\r\n$1

find 4: ^\}\r\n\r\n
repl 4: \}\r\n#endif\r\n\r\n
@recp
Copy link
Owner

recp commented Apr 13, 2020

Hi @tylo-work

Thank you for contributing to cglm 🤗


I don't know, this is a GOOD IDEA but brings a HUGE change to the design and code base and conflicts with the idea that provide both inline and compile version.

For instance users can use both glm_mat4_mul() (inline) and glmc_mat4_mul() (library-call) at the same time. glm_ is always inline (tries to be) and glmc_ is always library-called / compiled version. So there is no confusing.

I don't know, let's get more reviews, more feedbacks here.

@tylov
Copy link
Author

tylov commented Apr 14, 2020 via email

@recp
Copy link
Owner

recp commented Apr 14, 2020

I think this PR must stay open if you like to get more feedbacks from others. In the future we may want to bring this changes... Feel free to contribute cglm for other bug fixes and features... ;)

Some places may require some functions like glm_mat4_mul/glm_mul ... to be inline to get rid of function call overhead, in critical sections to get max performance. Some places may not require that to save binary size. Because inline functions probably will increase binary size...


Btw, I have a suggestion for new swizzle functions. Speed should be around as fast as yours, but this is more user friendly and flexible, imo:

glm_swizzle4(v1, “wxyy”, v2);

this is interesting approach. I must learn how this (swz[2] - 'x') & 3 works. Maybe ASCII codes are consecutive / followed each other.

My goal was to use SIMD here but I couldn't :'(, that would be fastest one.

Ps: Your following defines are wrong, should be (3, 2, 1, 0), although you won’t need them much if you use my swizzle functions 😉
#define GLM_WZYX GLM_SHUFFLE4(0, 1, 2, 3)
#define GLM_ZYX GLM_SHUFFLE3(0, 1, 2)

I think I have written tests for them. Nothing may be wrong, you may want to check GLM_SHUFFLE3 and GLM_SHUFFLE4 definitions.

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.

None yet

2 participants