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

Keep the usage of temporary EbmlDocVersion instances to init global semantic classes #228

Merged
merged 5 commits into from
Jan 28, 2024

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Jan 27, 2024

This will need an update on the libmatroska side before this is merged. After this MR it won't be possible to used temporary EbmlDocVersion anymore.
=> #228

We now have the EbmlId static+constepr and the Semantic and SemanticContext constexpr.

A test has been added to test the various issues found with EbmlDocVersion usage.

@robUx4 robUx4 added the bug label Jan 27, 2024
@robUx4 robUx4 added api-break breaks the API (e.g. programs using it will have to adjust their source code) abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) labels Jan 28, 2024
src/EbmlDummy.cpp Outdated Show resolved Hide resolved
@robUx4 robUx4 force-pushed the version_storage branch 2 times, most recently from 33d5a75 to e4c6baf Compare January 28, 2024 13:21
@mbunkus
Copy link
Contributor

mbunkus commented Jan 28, 2024

Thanks. Unfortunately I cannot really test it without a corresponding PR to libMatroska.

@robUx4
Copy link
Contributor Author

robUx4 commented Jan 28, 2024

Thanks. Unfortunately I cannot really test it without a corresponding PR to libMatroska.

It builds fine with Matroska-Org/libmatroska#168 for me

@mbunkus
Copy link
Contributor

mbunkus commented Jan 28, 2024

Ah right, I was only looking for a similar branch name. Sorry. Will test.

@mbunkus
Copy link
Contributor

mbunkus commented Jan 28, 2024

After pushing the latest merges this branch doesn't merge anymore. Can you please rebase? Thanks.

They can be accessed though the EbmlCallbacks of the given class.
This allows passing it in another constexpr constructor.
So it doesn't have to be a virtual class anymore and can be a real static constexp.
@robUx4
Copy link
Contributor Author

robUx4 commented Jan 28, 2024

Rebased this and Matroska-Org/libmatroska#168

@mbunkus mbunkus merged commit ff49764 into Matroska-Org:master Jan 28, 2024
15 checks passed
@neheb
Copy link
Contributor

neheb commented Jan 28, 2024

@robUx4

../src/EbmlHead.cpp:33:65: error: invalid escape sequence '\0' in an unevaluated string literal [clang-diagnostic-error]
   33 | DEFINE_EBML_MASTER_ORPHAN(EbmlHead, 0x1A45DFA3, false, "EBMLHead\0ratamapaga", AllEbmlVersions)

@neheb
Copy link
Contributor

neheb commented Jan 28, 2024

yeah.

[1/3] Compiling C++ object libebml.so.6.0.0.p/src_EbmlHead.cpp.o
FAILED: libebml.so.6.0.0.p/src_EbmlHead.cpp.o 
clang++ -Ilibebml.so.6.0.0.p -I. -I.. -fvisibility=hidden -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c++20 -O0 -g -fPIC '-DEBML_DLL_API=__attribute__((visibility("default")))' -MD -MQ libebml.so.6.0.0.p/src_EbmlHead.cpp.o -MF libebml.so.6.0.0.p/src_EbmlHead.cpp.o.d -o libebml.so.6.0.0.p/src_EbmlHead.cpp.o -c ../src/EbmlHead.cpp
../src/EbmlHead.cpp:33:65: error: invalid escape sequence '\0' in an unevaluated string literal
   33 | DEFINE_EBML_MASTER_ORPHAN(EbmlHead, 0x1A45DFA3, false, "EBMLHead\0ratamapaga", AllEbmlVersions)
      |                                                                 ^~
../ebml/EbmlElement.h:142:104: note: expanded from macro 'DEFINE_EBML_MASTER_ORPHAN'
  142 | #define DEFINE_EBML_MASTER_ORPHAN(x,id,infinite,name,versions)  DEFINE_xxx_MASTER_ORPHAN(x,id,infinite,name,versions,GetEbmlGlobal_Context)
      |                                                                                                        ^~~~
../ebml/EbmlElement.h:72:138: note: expanded from macro 'DEFINE_xxx_MASTER_ORPHAN'
   72 |     static constexpr const libebml::EbmlId Id_##x    {id}; static_assert(libebml::EbmlId::IsValid(Id_##x .GetValue()), "invalid id for " name ); \
      |                                                                                                                                          ^~~~
1 error generated.
[2/3] Compiling C++ object libebml.so.6.0.0.p/src_EbmlCrc32.cpp.o
FAILED: libebml.so.6.0.0.p/src_EbmlCrc32.cpp.o 
clang++ -Ilibebml.so.6.0.0.p -I. -I.. -fvisibility=hidden -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c++20 -O0 -g -fPIC '-DEBML_DLL_API=__attribute__((visibility("default")))' -MD -MQ libebml.so.6.0.0.p/src_EbmlCrc32.cpp.o -MF libebml.so.6.0.0.p/src_EbmlCrc32.cpp.o.d -o libebml.so.6.0.0.p/src_EbmlCrc32.cpp.o -c ../src/EbmlCrc32.cpp
../src/EbmlCrc32.cpp:31:53: error: invalid escape sequence '\0' in an unevaluated string literal
   31 | DEFINE_EBML_CLASS_ORPHAN(EbmlCrc32, 0xBF, "EBMLCrc32\0ratamadabapa", AllEbmlVersions)
      |                                                     ^~
../ebml/EbmlElement.h:143:94: note: expanded from macro 'DEFINE_EBML_CLASS_ORPHAN'
  143 | #define DEFINE_EBML_CLASS_ORPHAN(x,id,name,versions)            DEFINE_xxx_CLASS_ORPHAN(x,id,name,versions,GetEbmlGlobal_Context)
      |                                                                                              ^~~~
../ebml/EbmlElement.h:135:138: note: expanded from macro 'DEFINE_xxx_CLASS_ORPHAN'
  135 |     static constexpr const libebml::EbmlId Id_##x    {id}; static_assert(libebml::EbmlId::IsValid(Id_##x .GetValue()), "invalid id for " name ); \
      |                                                                                                                                          ^~~~
1 error generated.
ninja: build stopped: subcommand failed

Need clang++ CI to catch this.

@robUx4 robUx4 deleted the version_storage branch January 29, 2024 06:22
@robUx4
Copy link
Contributor Author

robUx4 commented Jan 29, 2024

We do build with clang on the macOS CI. And I don't think a \0 in a literal string is invalid in C++. It's not in C.

For the record this is used to detect usage of the source code in proprietary programs 😉

@robUx4
Copy link
Contributor Author

robUx4 commented Jan 29, 2024

Ah it's the concatenation with a \0 the leads to a problem. Still, it builds fine with AppleClang 14 or gcc 13. It's odd that all of a sudden this becomes an error and not just a warning.

@neheb
Copy link
Contributor

neheb commented Jan 29, 2024

To be fair, I ran into this error on Fedora 38.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break breaks the ABI (e.g. programs linked against the library have to be recompiled) api-break breaks the API (e.g. programs using it will have to adjust their source code) bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants