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

Cannot compile with Emscripten: incorrect forward declaration for std::vector #51

Closed
cschreib opened this issue Dec 22, 2021 · 4 comments · Fixed by #52
Closed

Cannot compile with Emscripten: incorrect forward declaration for std::vector #51

cschreib opened this issue Dec 22, 2021 · 4 comments · Fixed by #52

Comments

@cschreib
Copy link
Contributor

cschreib commented Dec 22, 2021

The file vector_fwd.hpp attempts to forward-declare std::vector, and uses compiler-specific ifdefs to specify the exact namespace inside std::. This currently does not work with Emscripten, which uses it's own version of libc++. Apparently, they use the std::__2 namespace, and not std::__1, which is what c4core currently assumes for libc++.

This can currently be fixed by modifying the code to:

#if defined(__EMSCRIPTEN__)
inline namespace __2 {
#else
inline namespace __1 {
#endif

An alternative, perhaps cleaner way, is to just use _LIBCPP_BEGIN_NAMESPACE_STD and _LIBCPP_END_NAMESPACE_STD?

But none of this is very stable/portable. I'm not sure this is worth the trouble, since <vector> will have to be included to actually use any of the functions declared in this header.

cschreib added a commit to cschreib/c4core that referenced this issue Dec 22, 2021
@biojppm
Copy link
Owner

biojppm commented Dec 22, 2021

Thanks for reporting. Yes, the existing vector_fwd.hpp approach is fragile. Your change is consistent with the existing code, and for me it is definitely ok.

If it matters, the current approach is meant to address a somewhat contradictory situation:

  • Users not wishing to include/use <vector> and/or <string> because of the heavy new/delete/exception machinery should still be able to use c4core.
  • Users using <vector> and/or <string> had a problem with the include order: there were constraints in that <c4/charconv.hpp> and then <vector>/<string> needed to come before any code using to_chars() for std::string or std::vector<char>.

Hence this _fwd approach which may be fragile and less that optimal, but in fact is currently working for the platforms/compilers/libraries of interest which are already quite a few. I had not tested yet with emscripten and that explains the problem you are seeing. Thanks for flagging it and thanks for the MR.

@cschreib
Copy link
Contributor Author

No worries! I haven't been in the shoes of one of these compile speed zealots yet, so it seems to me like a high maintenance price to pay to save a few milliseconds :) But I understand everyone's needs are different! And it's your maintenance time, not mine ;)

In this instance, perhaps a fallback to include <vector> if the STL cannot be identified would allow the library to still be used on platforms you haven't thought about. I'm thinking of this from the POV of someone using rapidyaml, which shouldn't require any platform-dependent stuff to do its job.

cschreib added a commit to cschreib/c4core that referenced this issue Dec 23, 2021
@cschreib
Copy link
Contributor Author

Thank you for merging the PR! Do you think you could upgrade the c4core version used by rapidyaml? Then I can get rid of my forks :)

@biojppm
Copy link
Owner

biojppm commented Dec 26, 2021

Already did!

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 a pull request may close this issue.

2 participants