Move enums from tags_int.hpp to tags.hpp #2276

kevinbackhouse · 2022-07-08T11:21:18Z

This moves the declarations of enums IfdId and SectionId from src/tags_int.hpp to include/exiv2/tags.hpp. I think it makes the code a bit cleaner because now the TagInfo struct can use those enums rather than int.

But maybe there's a reason why IfdId and SectionId shouldn't be included in the public interface that I'm not aware of? If so, I'll close this PR.

codecov · 2022-07-08T11:34:54Z

Codecov Report

Merging #2276 (3ef783e) into main (a22aea0) will decrease coverage by 0.00%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main    #2276      +/-   ##
==========================================
- Coverage   63.48%   63.47%   -0.01%     
==========================================
  Files         118      118              
  Lines       19598    19579      -19     
  Branches     9556     9549       -7     
==========================================
- Hits        12442    12428      -14     
+ Misses       5092     5091       -1     
+ Partials     2064     2060       -4

Impacted Files	Coverage Δ
include/exiv2/exif.hpp	`100.00% <ø> (ø)`
include/exiv2/tags.hpp	`33.33% <0.00%> (-66.67%)`	⬇️
src/canonmn_int.cpp	`71.55% <ø> (ø)`
src/casiomn_int.cpp	`6.89% <ø> (ø)`
src/cr2header_int.cpp	`68.42% <0.00%> (ø)`
src/cr2image.cpp	`19.48% <ø> (ø)`
src/fujimn_int.cpp	`100.00% <ø> (ø)`
src/minoltamn_int.cpp	`55.06% <ø> (ø)`
src/nikonmn_int.cpp	`57.14% <ø> (+0.03%)`	⬆️
src/olympusmn_int.cpp	`38.38% <ø> (ø)`
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a22aea0...3ef783e. Read the comment docs.

piponazo

I do not have anything against moving that enum to the API, and it makes sense to have it there so that TagInfo can reuse it. 👍

piponazo · 2022-07-11T16:24:22Z

include/exiv2/tags.hpp

@@ -47,18 +47,192 @@ struct EXIV2API GroupInfo::GroupName {
  std::string g_;  //!< Group name
 };

+//! Type to specify the IFD to which a metadata belongs
+enum IfdId {


[Suggestion] Did you think about making this enumeration a scoped enum (enum class)? I think it would help for code readability.

good suggestion!

I added a commit that does this. It's a big diff! I used a script to make most of the changes.

Pull request has been modified.

neheb · 2022-07-11T21:18:18Z

--- ./src/image_int.hpp	(original)
+++ ./src/image_int.hpp	(reformatted)
@@ -65,7 +65,7 @@
   explicit binaryToStringHelper(const Slice<T> myBuf) noexcept : buf_(myBuf) {
   }
-  friend std::ostream& operator<< <T>(std::ostream& stream, const binaryToStringHelper<T>& binToStr);
+  friend std::ostream& operator<<<T>(std::ostream& stream, const binaryToStringHelper<T>& binToStr);
   // the Slice is stored by value to avoid dangling references, in case we
   // invoke:

This is a difference between clang-format-12 and 14. Needs to be updated IMO.

src/tags.cpp

  if (!ti)
-    return sectionInfo[unknownTag.sectionId_].name_;
-  return sectionInfo[ti->sectionId_].name_;
+    return sectionInfo[static_cast<int>(unknownTag.sectionId_)].name_;


src/tags.cpp

-    return sectionInfo[unknownTag.sectionId_].name_;
-  return sectionInfo[ti->sectionId_].name_;
+    return sectionInfo[static_cast<int>(unknownTag.sectionId_)].name_;
+  return sectionInfo[static_cast<int>(ti->sectionId_)].name_;


piponazo

Thanks a lot! Specially for the additional effort for the final commit 👏

piponazo · 2022-07-12T06:27:23Z

This is a difference between clang-format-12 and 14. Needs to be updated IMO.

Few months ago when I introduced the clang-format workflow we discussed about this and we decided to use the clang-version coming with ubuntu-latest. However, if this is bothering you guys, I do not any objection to upgrade to clang-format-14. It can be done by just updating this file: .github/workflows/on_push_clang_format.yml. More information about the action can be found here:

https://github.com/DoozyX/clang-format-lint-action

kevinbackhouse · 2022-07-12T09:22:39Z

@piponazo @neheb: Regarding the clang-format version, I think the best choice is whatever people are most likely to have installed on their computers. I'm running Ubuntu 22.04, which comes with clang 14, but I expect most people are using Macs or Windows, and I don't know which version is the default on those platforms.

I can easily use a docker container to run version 12 if necessary, so I'm happy to work around this if 12 is the most popular choice.

jim-easterbrook · 2024-01-10T11:15:40Z

But maybe there's a reason why IfdId and SectionId shouldn't be included in the public interface that I'm not aware of? If so, I'll close this PR.

I just happened across this, making IfdId internal 11 years ago: https://dev.exiv2.org/issues/721

Move enums from tags_int.hpp to tags.hpp

3f7cb78

kevinbackhouse added this to the v1.00 milestone Jul 8, 2022

clang-format

62043c1

kevinbackhouse requested review from neheb, piponazo, hassec and kmilos July 8, 2022 12:35

piponazo previously approved these changes Jul 11, 2022

View reviewed changes

github-advanced-security bot found potential problems Jul 11, 2022

View reviewed changes

Convert IfdId and SectionId to enum classes.

3ef783e

kevinbackhouse force-pushed the TagInfo-enums branch from b7152ec to 3ef783e Compare July 11, 2022 21:38

piponazo approved these changes Jul 12, 2022

View reviewed changes

kevinbackhouse merged commit 0558662 into Exiv2:main Jul 12, 2022

kevinbackhouse deleted the TagInfo-enums branch July 12, 2022 09:23

nehaljwani mentioned this pull request Apr 30, 2023

Is a new release happening soon? #2406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move enums from tags_int.hpp to tags.hpp #2276

Move enums from tags_int.hpp to tags.hpp #2276

kevinbackhouse commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

piponazo left a comment

piponazo Jul 11, 2022

kevinbackhouse Jul 11, 2022

kevinbackhouse Jul 11, 2022

neheb commented Jul 11, 2022

piponazo left a comment

piponazo commented Jul 12, 2022

kevinbackhouse commented Jul 12, 2022

jim-easterbrook commented Jan 10, 2024

Move enums from tags_int.hpp to tags.hpp #2276

Move enums from tags_int.hpp to tags.hpp #2276

Conversation

kevinbackhouse commented Jul 8, 2022 • edited Loading

codecov bot commented Jul 8, 2022 • edited Loading

Codecov Report

piponazo left a comment

Choose a reason for hiding this comment

piponazo Jul 11, 2022

Choose a reason for hiding this comment

kevinbackhouse Jul 11, 2022

Choose a reason for hiding this comment

kevinbackhouse Jul 11, 2022

Choose a reason for hiding this comment

neheb commented Jul 11, 2022

piponazo left a comment

Choose a reason for hiding this comment

piponazo commented Jul 12, 2022

kevinbackhouse commented Jul 12, 2022

jim-easterbrook commented Jan 10, 2024

kevinbackhouse commented Jul 8, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading