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

clang-tidy: fix missing special member func #173

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

neheb
Copy link
Contributor

@neheb neheb commented Feb 1, 2024

Found with: cppcoreguidelines-special-member-functions

Replaced several functions with using declarations and added missing stuff elsewhere.

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.

There's way too many things happening in a single commit.

matroska/KaxDefines.h Show resolved Hide resolved
@neheb neheb force-pushed the 3 branch 2 times, most recently from c9b80c7 to 6724dce Compare February 2, 2024 07:21
@neheb
Copy link
Contributor Author

neheb commented Feb 2, 2024

Split into three commits. Hopefully simpler now.

@neheb neheb force-pushed the 3 branch 2 times, most recently from 4a1a8c4 to 263d990 Compare February 2, 2024 08:32
matroska/KaxDefines.h Outdated Show resolved Hide resolved
matroska/KaxDefines.h Outdated Show resolved Hide resolved
@@ -337,8 +337,6 @@ class MATROSKA_DLL_API KaxBlockBlob {

DECLARE_MKX_BINARY_CONS(KaxBlockVirtual)
public:
~KaxBlockVirtual() override;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should make no difference doing that here or in the DECLARE_MKX_BINARY_CONS macro above.

I don't understand the commit description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simpler to do it in the macro.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not up to the macro to decide if the child class needs an overriden destructor or not. The macro is only to have a custom CONStructor. The form of the desctructor is up to each class.

It is currently possible to factorize this. But since this will be part of the new API, if ever need to move the destructor out of the macro, we'll be screwed.

Also this commit changes the desctructor and a copy constructor. They should be split. And I wonder why the copy contructor is not allowed.

neheb added 3 commits February 2, 2024 17:22
Found with: cppcoreguidelines-special-member-functions

A using declaration allows to get rid of the warning while avoiding
implementing a bunch of stuff to satisfy it.

Signed-off-by: Rosen Penev <[email protected]>
Some of these destructors are not default, so we can't do that here. We
must change all functions to be out of line.

Slightly simplifies macro users.

Signed-off-by: Rosen Penev <[email protected]>
Found with: cppcoreguidelines-special-member-functions

Mostly adding deleted functions. Added a using declaration in one case
for simplicity.

Signed-off-by: Rosen Penev <[email protected]>
@@ -83,6 +83,8 @@ class MATROSKA_DLL_API SimpleDataBuffer : public DataBuffer {
{}
~SimpleDataBuffer() override = default;

SimpleDataBuffer& operator=(const SimpleDataBuffer &) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this forbidden ? Should it be mixed with the previous commit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the error is that the operator is missing. I deleted it only because it does not cause a compile error.

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.

Still needs some more commit splitting and explanation.
I'm also against the API change in the CONS macros.

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