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

Use the strict access API #135

Merged
merged 7 commits into from
Dec 18, 2023
Merged

Use the strict access API #135

merged 7 commits into from
Dec 18, 2023

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Oct 16, 2022

So far the code was built without the define. But since we can change the API for 2.0 we should switch to a stricter API that makes things less public.

This requires libmatroska to merge Matroska-Org/libmatroska#95

We might be able to add some constexpr here and there despite using methods.

@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 Oct 16, 2022
@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

This PR completely breaks compilation of MKVToolNix. I'm still evaluating if I can convert all of that to only using the strict API.

In the meantime, please take a look at this new warning that clang emits now:

In file included from src/common/debugging.cpp:19:
lib/libebml/ebml/EbmlDummy.h:59:9: warning: binding dereferenced null pointer to reference has undefined behavior [-Wnull-dereference]
        EBML_CONCRETE_DUMMY_CLASS(EbmlDummy)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/mosu/prog/video/mkvtoolnix/lib/libebml/ebml/EbmlElement.h:194:69: note: expanded from macro 'EBML_CONCRETE_DUMMY_CLASS'

Thanks.

@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

Another issue that needs a fix:

EbmlElement defines a function called Context(). EbmlMaster defines a private member variable Context. This makes it impossible or at least very, very difficult to call the Context() function on an EbmlMaster and derived classes. MKVToolNix does indeed need this.

@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

And while we're at it, please add a GetId() const member function to EbmlElement that does the same as operator EbmlId(). Using a member function instead of all those bloody operators is much cleaner style & muss less confusion for people not as familiar with the code.

Copy link
Contributor

@mbunkus mbunkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've successfully converted MKVToolNix to using only the strict API stuff (a lot via the provided macros to still compile with old library versions which don't provide all the required public functions directly, but sometimes via indirections such as Generic()).

What I'd like to see before merging are the things I've already pointed out:

  1. Fix the undefined behavior the compiler warns about
  2. Resolve the ambiguity between the Context() function in EbmlElement and the Context member variable in EbmlMaster (e.g. by renaming the function to GetContext() which would be more in line with the other Get…() functions)
  3. Provide proper functions for all operator… conversion operator functions

@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

Rebased onto current master

@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

I'm working on changes on top of this branch that address the points I made.

@robUx4
Copy link
Contributor Author

robUx4 commented Oct 16, 2022

OK, not sure when I'll have time to work on this, so feel free to make any progress.

There are some macro helpers that are useful to switch from strict to non-strict, so you probably use that for the time being so your code compile with both variants.

@mbunkus
Copy link
Contributor

mbunkus commented Oct 16, 2022

I've just pushed a handful of commits that implement everything I wanted. Please review when you find the time.

I'd also like to push a cosmetics commit fixing indentation (we had this at 2 spaces at one point, but now it's becoming a really bad mixture of 2, 4 and other amounts of spaces again), but that'll make the diff way less obvious. Therefore I'm holding off on it until we get this PR merged.

robUx4 and others added 6 commits December 17, 2023 15:21
We might be able to add some more constexpr in there.
All the members become private.

This requires libmatroska to merge Matroska-Org/libmatroska#95
Also removes the EBML_CONCRETE_DUMMY_CLASS macro as it's only used by
EbmlDummy anyway.
…bmlCallbacks

This was already possible by using the operator EbmlCallbacks(), but
that isn't really idiomatic C++ style.

The operator is kept for backwards compatibility.
This was already possible by using the operator EbmlId(), but that
isn't really idiomatic C++ style.

The operator is kept for backwards compatibility.
@robUx4
Copy link
Contributor Author

robUx4 commented Dec 17, 2023

The changes look fine and it does build with EBML_STRICT_API

@mbunkus
Copy link
Contributor

mbunkus commented Dec 17, 2023

Trying to build MKVToolNix with this branch & the latest master from libMatroska fails during the linking stage:

ld.lld: error: undefined symbol: libebml::EbmlDummy::ClassId()
>>> referenced by ebml.h:124 (src/common/ebml.h:124)
>>>               r_matroska.o:(kax_reader_c::read_headers_internal()) in archive src/input/libmtxinput.a
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)

This is due to the function not having an implementation anymore. Easily fixed by replacing the declaration with:

    static const EbmlId & ClassId() { return DummyRawId; }; 

Otherwise I'm still fine with the changes. Thanks!

@robUx4
Copy link
Contributor Author

robUx4 commented Dec 17, 2023

Thanks for testing (after this long there was invitably things that might break). It should work with the latest push.

@mbunkus
Copy link
Contributor

mbunkus commented Dec 17, 2023

Works, thanks!

@robUx4 robUx4 merged commit c6955f4 into master Dec 18, 2023
20 checks passed
@robUx4 robUx4 deleted the strict-access branch December 18, 2023 06:24
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants