-
Notifications
You must be signed in to change notification settings - Fork 278
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 unused var from deleted stuff #2176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2176 +/- ##
=======================================
Coverage 63.29% 63.29%
=======================================
Files 99 99
Lines 19569 19569
Branches 9554 9554
=======================================
Hits 12386 12386
Misses 5112 5112
Partials 2071 2071
Continue to review full report at Codecov.
|
Signed-off-by: Rosen Penev <rosenp@gmail.com>
@@ -79,7 +79,7 @@ class EXIV2API BasicIo { | |||
@return Number of bytes written to IO source successfully;<BR> | |||
0 if failure; | |||
*/ | |||
virtual size_t write(const byte *data, size_t wcount) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O_O ... is not this controlled by clang-format? I am pretty sure the is something in clang-format to control the indentation of &
and *
in function parameters.
Was this something that you changed manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format did this, yes. In the current setup, clang-format decides the order based on the most used order. Probably should disable DerivePointerAlignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, probably we should use something like:
# Force pointers to the type for C++.
DerivePointerAlignment: false
PointerAlignment: Left
To enforce the same style over the whole project. @hassec what's your opinion on this point?
For me it is fine to accept this PR as it is, and then create another PR to address those clang-format changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree with you @piponazo 👍
And yes, I also think we can move this forward independently and fix clang-format
in a different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointer alignment should be left, yes.
@@ -132,12 +132,10 @@ class EXIV2API IptcDataSets { | |||
static constexpr uint16_t Preview = 202; | |||
//@} | |||
|
|||
//! Prevent construction: not implemented. | |||
IptcDataSets() = delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning to allow the creation of this class now? This change looks out of the scope of the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A default constructor does not get generated here. I removed it as it's a bit misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh god, you're totally right. I am too far from coding lately and I forget these details about C++ 😩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leaving the explicit = delete
doesn't hurt though and helps a lot when reading the code as one then doesn't need to worry about knowing the details of when special member functions are implicitly declared...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that sometimes being explicit about certain things is better for code readability, but it is difficult to define where to draw the line. On the other hand, less code is almost always the way to go for me. In this case, I think it is fine to remove those lines, if somebody try to create an instance of the class with the default constructor the compiler will complain anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: Rosen Penev rosenp@gmail.com