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

remove deleted functions #2492

Merged
merged 6 commits into from
Feb 6, 2023
Merged

remove deleted functions #2492

merged 6 commits into from
Feb 6, 2023

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Feb 2, 2023

No description provided.

@ghost
Copy link

ghost commented Feb 2, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #2492 (3a8e75d) into main (0d353ac) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2492      +/-   ##
==========================================
- Coverage   62.12%   62.04%   -0.08%     
==========================================
  Files         121      102      -19     
  Lines       22827    22768      -59     
  Branches    11206    11206              
==========================================
- Hits        14181    14127      -54     
+ Misses       6439     6434       -5     
  Partials     2207     2207              
Impacted Files Coverage Δ
app/actions.hpp 100.00% <ø> (ø)
include/exiv2/basicio.hpp 100.00% <ø> (+9.09%) ⬆️
include/exiv2/datasets.hpp 100.00% <ø> (ø)
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/matroskavideo.hpp 56.25% <ø> (ø)
include/exiv2/pgfimage.hpp 100.00% <ø> (ø)
include/exiv2/tags.hpp 0.00% <ø> (ø)
include/exiv2/types.hpp 82.85% <ø> (ø)
include/exiv2/value.hpp 85.52% <ø> (-0.39%) ⬇️
src/basicio.cpp 51.73% <ø> (+0.06%) ⬆️
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@neheb
Copy link
Collaborator Author

neheb commented Feb 2, 2023

git grep =\ delete | grep \(\) | wc -l
7

is extremely suspicious. Anyone have any idea?

@piponazo
Copy link
Collaborator

piponazo commented Feb 2, 2023

git grep =\ delete | grep \(\) | wc -l
7

is extremely suspicious. Anyone have any idea?

About what? It looks like many of these delete constructors were added by you in the past:
b53ed72

@neheb
Copy link
Collaborator Author

neheb commented Feb 2, 2023

yeah but deleted destructors seem weird. You can't construct something like that. I did such a thing and tested that compilation works. That's about it.

@neheb
Copy link
Collaborator Author

neheb commented Feb 3, 2023

googling indicates these things are basically no op. Removed.

//! @name NOT Implemented
//@{
//! Copy constructor
BmpImage(const BmpImage&) = delete;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to be more careful with these changes. Even in 0.27 we were "not implementing" the copy constructor & assignment in purpose, with the aim of making some classes not copyable. This would be an important API change, and probably we do not want to make such objects copyable anyways.

The deletion of the default destructors looks fine to me though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for all the removals here, the base class has deleted copy constructor and operator= which is inherited by the derived classes. All of them are not copyable unless such a thing is implemented per class.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, I did not notice the Image class already was deleting the copy constructor and operator.

@neheb
Copy link
Collaborator Author

neheb commented Feb 5, 2023

ping @kmilos

The inherited Image class already has these same deleted functions.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
This is a holdover from C++98. No need anymore

Signed-off-by: Rosen Penev <rosenp@gmail.com>
@neheb neheb merged commit e309680 into Exiv2:main Feb 6, 2023
@neheb neheb deleted the deff branch February 6, 2023 06:38
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