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

Avoid compiler warnings #152

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Avoid compiler warnings #152

merged 2 commits into from
Dec 13, 2023

Conversation

mbunkus
Copy link
Contributor

@mbunkus mbunkus commented Dec 12, 2023

Please read the commit messages. They explain in great detail what the warnings & fixes are about.

Using an `unsigned int` in the function is safe as all values assigned
to it are > 0.

Furthermore, logically the function cannot return a value < 1,
therefore it should return an `unsigned int` or maybe
`std::size_t` (as it deals with calculating a number of bytes a
certain element is long) instead of an `int`, but that would change
the API & ABI.
…classes

In the base class `EbmlElement` there's the `virtual operator const
EbmlId &`. This one is overridden in derived classes in
libmatroska (and in `EbmlDummy` as well) in order to return the EBML
ID belonging to the concrete element's class in libmatroska.

The classes in libmatroska don't derive directly from
EbmlElement. Instead there's base-type-specific intermediate
class (`EbmlUInteger`, `EBMLSInteger`, `EBMLString`,
`EBMLUnicodeString`, `EbmlFloat`, `EbmlDate`). For example:

`EbmlElement` ← `EbmlUInteger` ← `KaxTrackNumber`

Some of these intermediate classes provide some non-`virtual`
`operator`s themselves (e.g. an `operator std::uint64_t` for
`EbmlUInteger`) but don't override the `virtual operator const
EbmlId &` from the base class, `EbmlElement`.

For regular functions this is a common mistake made by programmers,
assuming that even though they're leaving out the `virtual` key word
in the derived class the function from the derived class overrides the
`virtual` function from the base class. However, due to different
signatures they don't override; instead you now have two different
functions in the same class.

This is where the compiler comes in: `g++` 13.2.x emits a warning
about this if the category `overloaded-virtual` is enabled.

The fix is to tell the compiler that we're doing what we intend to
here: we do not want to override the `virtual operator` from the base
class; instead we intentionally introduce further, different
non-`virtual` `operators`. We do this with a `using Base::operator …`
directive.
@mbunkus mbunkus requested a review from robUx4 December 12, 2023 21:35
Copy link
Contributor

@robUx4 robUx4 left a comment

Choose a reason for hiding this comment

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

Are these meant for 1.x or 2.x ? If it's just 2.x we don't care about API breakage.

@mbunkus
Copy link
Contributor Author

mbunkus commented Dec 13, 2023

I had only intended them for v2.x. They're just warnings, after all, and I don't plan on long-term maintenance for v1.x. Neither patch should break API or ABI, though.

@mbunkus mbunkus merged commit 0100bd8 into master Dec 13, 2023
20 checks passed
@mbunkus mbunkus deleted the avoid-compiler-warnings branch December 13, 2023 17:15
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 this pull request may close these issues.

2 participants