-
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
find changes #2462
find changes #2462
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2462 +/- ##
=======================================
Coverage 62.08% 62.09%
=======================================
Files 121 121
Lines 22830 22831 +1
Branches 11212 11212
=======================================
+ Hits 14175 14176 +1
Misses 6448 6448
Partials 2207 2207
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
82ad0c0
to
81d7de1
Compare
hmmm should the find() template be public API? |
73a17b3
to
c9a79e2
Compare
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.
In general the changes look good (less verbose code). The only thing I would change is to use auto *
instead of auto
to make more evident that Exiv2::find
is returning a T *
. I'll leave the decision up to you.
@@ -317,7 +317,7 @@ void readWriteEpsMetadata(BasicIo& io, std::string& xmpPacket, NativePreviewList | |||
#ifdef DEBUG | |||
EXV_DEBUG << "readWriteEpsMetadata: First line: " << firstLine << "\n"; | |||
#endif | |||
bool matched = std::find(epsFirstLine.begin(), epsFirstLine.end(), firstLine) != epsFirstLine.end(); | |||
auto matched = Exiv2::find(epsFirstLine, firstLine); |
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.
[discussion] auto
here is taking the value of T *
. Even though it is not needed, I normally find more readable to use auto *
to declare the intention of using pointers. What do you think about this?
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 don't like auto* mostly because of const. Eg.
auto = const char*
auto* = char*
const auto = const char* const
const auto* = const char*
Regular auto makes more sense IMO.
Pull request has been modified.
std::find in C++20 can use ranges, which is equivalent here. Less error prone. Namespace is properly to avoid any conflicts with std::find or others Signed-off-by: Rosen Penev <rosenp@gmail.com>
The latter is simpler. Signed-off-by: Rosen Penev <rosenp@gmail.com>
👇 Click on the image for a new way to code review
Legend |
ping @kmilos |
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
Not sure if better.