bitsery
bitsery copied to clipboard
fraillt
Fix compilation errors in clang 17
Could you replace <cstdint> with stdint.h as described on my issue? I had to do that when compiling via libclang (18). cstdint isn't guaranteed to export these types other than in std::.
Does your distribution not ship the global definitions in the cstdint header? Could I get some details on your setup? If you mean it in the "this is not necessarily guaranteed but works 100% of the time" kind of thing, then I think I'll let the maintainer of this repo make the call
I'm using a fork of https://github.com/pybind/pybind11_mkdoc on Ubuntu 24.04 with clang-tools-18 installed from apt.
I get errors about uint8_t not being known unless I patch the call to libclang to additionally pass -include stdint.h (which implicitly includes that header everywhere).
Interesting, I'll see if I can reproduce this, it's the first time I see a distribution make this dumb of a decision! In any case, consider it done (tomorrow probably)!
Thanks for pinging ;) It's not dead, I just consider it feature complete, so I don't actively work on it anymore, but small improvements, like yours, are always welcome;)
Hi! Sorry, you just caught me on vacation, I committed the change from cstdint to stdint.h, if you can resolve the small conflict and squash the changes, that would be ideal, if not I'll do it when I have a sec.
I completely get that if there's nothing to improve, there's simply nothing to change! I have a question, would you be open to getting C++23 code for flat_maps and stuff like that? I imagine that at some point we'll start using it and will want to be able to serialize it.
I wouldn't mind to accept these changes as part of extensions, there already exists some extensions for variant and optional which both are C++17. I'm a bit afraid to introduce new C++ standard to core bitsery functionality...
Btw, at the moment, I don't have access to my computer either, maybe at the end of next week I'll be able to sit down and resolve conflicts..
There don't seem like to be any conflicts? CI failure is unrelated and on master too. And squashing can be done from the GitHub UI when merging by @fraillt.
I wouldn't mind to accept these changes as part of extensions, there already exists some extensions for variant and optional which both are C++17. I'm a bit afraid to introduce new C++ standard to core bitsery functionality...
Absolutely no need to use them in the main core functionality, I was just thinking of adding extensions for those, as you say. It can all be disabled based on what C++ version you're using to compile with.
I think it is fine and very reasonable for bitsery to support C++11, as looking at the last surveys by the committee (which will have selection bias) still show that more than 25% of users still can't fully use C++17...
Btw, at the moment, I don't have access to my computer either, maybe at the end of next week I'll be able to sit down and resolve conflicts..
That's fine, I'm not in a rush! I have a bit of time, let me see if I can rebase stuff...
@VelocityRa I had made a quick merge from my phone, previously it complained. Please check that it indeed now uses stdint.h headers
Help me to understand this change... the way I understand, stdint.h file from C standart library (C99), while cstdint> file is from C++ standart library (C+11).
My first thought is that cstdint should be preferred.
Maybe the issue is with your LLVM configuration? https://stackoverflow.com/questions/76798324/error-with-include-cstdint-using-clang-17
On the other hand, maybe this is exactly what you need (use LLVM without C++ std library)?
The C++ standard technically only guarantees fixed size ints to be on the std namespace with the cstdint header, while the C header that is kept for retrocompatibility only guarantees the symbols on the global namespace. In practice, I have not ever seen a single case where the cstdint header didn't also include the global symbols, but @VelocityRa insisted on keeping the C headers. I believe the issue you linked is the most likely cause of what he reported, so I think that this change can be reverted. There's also the point of the maybe_unused attribute being used, which technically could be a problem as it is not included by default in C++11 (but most implementations have support for more modern attributes in older standards)
That link seems unrelated and I do compile with STD as you normally would.
The errors were:
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:54:13: error: unknown type name 'uint8_t'
etc
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:138:62: error: use of undeclared identifier 'uint8_t'
I also got this issue on GCC 13 a few days ago:
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h: In function ‘void bitsery::details::readSize(Reader&, size_t&, size_t, TCheckMaxSize)’:
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:54:13: error: ‘uint8_t’ was not declared in this scope
etc
vcpkg_installed/x64-linux/include/bitsery/details/adapter_common.h:36:1: note: ‘uint8_t’ is defined in header ‘<cstdint>’; did you forget to ‘#include <cstdint>’?
etc
So it's not just a clang/llvm issue.
And including stdint.h fixes it (I just do it by passing -include stdint.h to the compiler currently, which is an ugly workaround).
I know that this will be annoying, but could it be possible for you to test the latest version of the repo? I needed to include the cstdint headers and that was merged earlier.
This way we can absolutely be sure that this is indeed needed. Also, if you could provide the output from make with the -n flag so it gives the compiler command being used where it fails, we could maybe get some extra info on this odd setup. If this still happens, I will take action to actually reproduce this because I simply find it really really hard to believe that Ubuntu specifically would choose to be pedantic and not include the global definitions for the obscure reason that the standard does not guarantee it.
Also, I'd try to reinstall the standard library you're using just to be safe.
Sorry if I'm asking for a lot of actions...
I did some testing using Docker. There's the script that I made.
FROM ubuntu:latest
RUN apt update
RUN apt install git cmake libgmock-dev -y
WORKDIR /testing
RUN git clone https://github.com/fraillt/bitsery.git
WORKDIR bitsery
RUN apt install clang-17 -y
RUN update-alternatives --install /usr/bin/c++ c++ /usr/bin/clang++-17 100
RUN update-alternatives --install /usr/bin/cc cc /usr/bin/clang-17 100
RUN cmake -S . -B build -DBITSERY_BUILD_TESTS=ON -DBITSERY_BUILD_EXAMPLES=ON
RUN cmake --build build
RUN ctest --test-dir build
Put this in dockerfile and run with docker build -t test-bitsery .
The outcome is that it works just fine with clang-17 using bitsery master branch with no warnings...
Oh I just saw https://github.com/fraillt/bitsery/pull/106/files... it was merged a year ago but the latest release (v5.2.3) doesn't include it.
I don't use master, I use the vcpkg port which uses tagged releases.
I can't test right now but that's most likely the cause. It also explains why -include stdint.h fixed it (cstdint would fix it too).
@fraillt After this PR (with the stdint.h change removed by @Destroyerrrocket) is merged, could you make a new release, please?
Also updated the vcpkg port: https://github.com/microsoft/vcpkg/pull/40177 Thank you both :)
Thank you so much for the work, @fraillt ! ^-^
Would it be possible for you to update the Conan version? If it's too much work, maybe I can do that myself
I'm not familiar with Conan, maybe you could do it? :) I know that there exist Conan package for bitsery, but I'm not sure who was that nice person that made this happen :)