Remove unnecessary field

[kevinbackhouse]

Remove CiffDirectory::cc_. It's a raw pointer, so it's ownership is unclear. There also doesn't seem to be any reason to store it in the object.

[kmilos]

Looks like there was a bug hiding in there as well...

[kevinbackhouse]

I'm very puzzled why it's only failing on some of the test runs.

[kmilos]

I think there are some 30+ new lines not parsed before!

/Users/runner/work/exiv2/exiv2/test/data/test_reference_files/crw-test.out: 269 lines
/Users/runner/work/exiv2/exiv2/test/tmp/crw-test.out: 301 lines

[kmilos]

I'm very puzzled why it's only failing on some of the test runs.

Not all jobs run all the tests, e.g. some unit tests or NLS tests are skipped on some...

[neheb]

I mean, there's recursion going on. That's probably why it's a member of the class.

[neheb]

locally these are the new lines:

> Exif.CanonCs.Macro                           Short       1  Off
> Exif.CanonCs.Selftimer                       Short       1  Off
> Exif.CanonCs.Quality                         Short       1  RAW
> Exif.CanonCs.FlashMode                       Short       1  Off
> Exif.CanonCs.DriveMode                       Short       1  Single / timer
> Exif.CanonCs.FocusMode                       Short       1  AI servo AF
> Exif.CanonCs.RecordMode                      Short       1  CRW+THM
> Exif.CanonCs.ImageSize                       Short       1  Large
> Exif.CanonCs.EasyMode                        Short       1  Manual
> Exif.CanonCs.DigitalZoom                     Short       1  None
> Exif.CanonCs.Contrast                        Short       1  Normal
> Exif.CanonCs.Saturation                      Short       1  Normal
> Exif.CanonCs.Sharpness                       Short       1  Normal
> Exif.CanonCs.ISOSpeed                        Short       1  100
> Exif.CanonCs.MeteringMode                    Short       1  Evaluative
> Exif.CanonCs.FocusType                       Short       1  Auto
> Exif.CanonCs.AFPoint                         Short       1  Center
> Exif.CanonCs.ExposureProgram                 Short       1  Program (P)
> Exif.CanonCs.LensType                        Short       1  n/a
> Exif.CanonCs.Lens                            Short       3  7.1 - 21.3 mm
> Exif.CanonCs.MaxAperture                     Short       1  F2.9
> Exif.CanonCs.MinAperture                     Short       1  F8
> Exif.CanonCs.FlashActivity                   Short       1  Did not fire
> Exif.CanonCs.FlashDetails                    Short       1  
> Exif.CanonCs.FocusContinuous                 Short       1  Single
> Exif.CanonCs.AESetting                       Short       1  Normal AE
> Exif.CanonCs.ImageStabilization              Short       1  (65535)
> Exif.CanonCs.DisplayAperture                 Short       1  0
> Exif.CanonCs.ZoomSourceWidth                 Short       1  2272
> Exif.CanonCs.ZoomTargetWidth                 Short       1  2272
> Exif.CanonCs.SpotMeteringMode                Short       1  Center
> Exif.Photo.ColorSpace                        Short       1  Uncalibrated

how strange...

edit: analyzing further I see in the reference

Set Exif.Photo.ColorSpace "65535" (Short)

but no corresponding Exif.Photo.ColorSpace entry. With this PR it's

Exif.Photo.ColorSpace                        Short       1  Uncalibrated

is this a bugfix?

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.71%. Comparing base (46925ed) to head (813af22).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crwimage_int.cpp	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3137      +/-   ##
==========================================
+ Coverage   64.65%   64.71%   +0.06%     
==========================================
  Files         104      104              
  Lines       22167    22164       -3     
  Branches    10852    10843       -9     
==========================================
+ Hits        14331    14344      +13     
+ Misses       5596     5590       -6     
+ Partials     2240     2230      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kmilos]

is this a bugfix?

Looks like it. Members m_ and components_ already keep the state in the recursion?

[kevinbackhouse]

I need to do a bit more investigation to understand why this changes the results. At least it's not non-deterministic, which is what it looked like to me when only half the test runs failed.

I suspect it's something like it's changed the order of the elements in the components list, so when it searches for the first match it's getting a different answer.

[kmilos]

Well, the test reference output was wrong to begin with - looks like we were actually deleting stuff (existing data was no longer in the output) after all the "set" actions, so this was definitely a bug...

Pull request has been modified.

[kevinbackhouse]

I found the difference. Previously, this code was never executed:

exiv2/src/crwimage_int.cpp

Lines 568 to 571 in d7ac558

	// Tag doesn't exist yet, add it
	m_ = std::make_unique<CiffEntry>(crwTagId, tag());
	cc_ = m_.get();
	add(std::move(m_));

It wasn't called because cc_ wasn't null. But I think the value of cc_ that it returned was wrong because cc_->tagId() didn't match the requested value.

After this PR, the test calls that code exactly once. It creates a new CiffEntry with the correct tagId.

[neheb]

Now that I look at this, why is m_ a member of the class? It's only used in this function

--- a/src/crwimage_int.cpp
+++ b/src/crwimage_int.cpp
@@ -552,9 +552,9 @@ CiffComponent* CiffDirectory::doAdd(CrwDirs& crwDirs, uint16_t crwTagId) {
       }
     if (!cc) {
       // Directory doesn't exist yet, add it
-      m_ = std::make_unique<CiffDirectory>(dir.dir, dir.parent);
-      cc = m_.get();
-      add(std::move(m_));
+      auto m = std::make_unique<CiffDirectory>(dir.dir, dir.parent);
+      cc = m.get();
+      add(std::move(m));
     }
     // Recursive call to next lower level directory
     return cc->add(crwDirs, crwTagId);
@@ -568,9 +568,9 @@ CiffComponent* CiffDirectory::doAdd(CrwDirs& crwDirs, uint16_t crwTagId) {
     }
   if (!cc) {
     // Tag doesn't exist yet, add it
-    m_ = std::make_unique<CiffEntry>(crwTagId, tag());
-    cc = m_.get();
-    add(std::move(m_));
+    auto m = std::make_unique<CiffEntry>(crwTagId, tag());
+    cc = m.get();
+    add(std::move(m));
   }
   return cc;
 }  // CiffDirectory::doAdd

tests succeed.

[kmilos]

@mergify backport 0.28.x

[mergify]

backport 0.28.x

✅ Backports have been created

• #3139 Remove unnecessary field (backport #3137) has been created for branch 0.28.x but encountered conflicts