felixguendling / cista

Cista is a simple, high-performance, zero-copy C++ serialization & reflection library.
https://cista.rocks
MIT License
1.78k stars 113 forks source link

Build fails with the "use of undeclared identifier 'mi_malloc_aligned'" error #154

Closed tocic closed 1 year ago

tocic commented 1 year ago
  1. mkdir test && cd test
  2. wget https://github.com/felixguendling/cista/releases/download/v0.10/cista.h
  3. echo '#include "cista.h"' > main.cpp
  4. clang++ -std=c++17 main.cpp

leads to

In file included from main.cpp:1:
./cista.h:2949:9: error: use of undeclared identifier 'mi_malloc_aligned'
        CISTA_ALIGNED_ALLOC(ALIGNMENT, static_cast<size_t>(size)));
        ^

clang version 14.0.6 Target: x86_64-pc-linux-gnu

Seems like this line is missing from the amalgamated file.

felixguendling commented 1 year ago

Will look into it. For now: does it help if you add #include "mimalloc.h" before including cista.h? I guess you want to use cista in a project that uses mimalloc? Otherwise, I would remove mimalloc.h from the headers that are visible when compiling the your project.

tocic commented 1 year ago

Yes, adding #include "mimalloc.h" anywhere before the mi_malloc_aligned use leads to successfull compilation, but you also need to link to the library (-lmimalloc) in order to get rid of the "undefined reference" errors.

No, I don't need mimalloc for now, just wanted to compile the simple example from the README.

felixguendling commented 1 year ago

Currently, the assumption is that if you have the mimalloc.h available as an include, then you want to use mimalloc. Maybe this is not the right way to do it and cista should have a flag CISTA_WITH_MIMALLOC=1 to indicate that you want to use mimalloc.

tocic commented 1 year ago

In my case, the mimalloc package is just a dependency of another, unrelated project, mold, so I definitely didn't want to use mimalloc here. I think a more correct assumption would be that if you have already included the mimalloc.h header in your project, then you want to use it in cista. In this case, you should check for the presence of some predefined macro like MI_MALLOC_VERSION instead of the header existance. Your suggestion to explicitly request mimalloc works too.

Anyway, the problem at hand is that in the amalgamated file after checking for the existence of the header, we assume that it is already included. I think the easiest solution is to change the aforementioned line as follows:

25c25
< #include "mimalloc.h"
---
> #include <mimalloc.h>

Now, uniter.cc ignores this line and the amalgamated file will change as follows:

1978c1978
< // mimalloc.h: basic_ios::clear: iostream error
---
> #include <mimalloc.h>

But then you still need to link to the library in order to use its functions.

felixguendling commented 1 year ago

I think the CISTA_WITH_MIMALLOC gives the use more options to have mimalloc installed but not use it. #159 fixes it. Please reopen / create a new issue if the change causes new issues. Thank you very much for bringing up this issue!

tocic commented 1 year ago

Thank you for the quick resolution! Do you plan to update the release assets soon or is it better to manually build cista.h from the top master's commit?

felixguendling commented 1 year ago

Every PR build produces a cista.h artifact that can be downloaded from the "Actions" tab (requires login) if no release is available.

The new v0.11 contains the changes from this PR.