Improvements in the Error class #2141

piponazo · 2022-03-10T21:34:08Z

This PR is resuming the work that @D4N started few years ago in #685. By including these changes we should be able to fix the issues reported in #2116 (@clanmills I would appreciate if you could give it a try).

clanmills · 2022-03-10T21:40:10Z

WOW. I'll look at this tomorrow. Thanks.

codecov · 2022-03-10T21:40:16Z

Codecov Report

Merging #2141 (fa24fb2) into main (5d08bb9) will decrease coverage by 0.00%.
The diff coverage is 30.28%.

@@            Coverage Diff             @@
##             main    #2141      +/-   ##
==========================================
- Coverage   63.32%   63.31%   -0.01%     
==========================================
  Files          97       97              
  Lines       19137    19152      +15     
  Branches     9717     9724       +7     
==========================================
+ Hits        12118    12127       +9     
- Misses       4751     4762      +11     
+ Partials     2268     2263       -5

Impacted Files	Coverage Δ
src/basicio.cpp	`50.18% <0.00%> (ø)`
src/canonmn_int.cpp	`75.14% <0.00%> (ø)`
src/cr2image.cpp	`18.98% <0.00%> (-0.50%)`	⬇️
src/crwimage.cpp	`67.90% <0.00%> (-0.85%)`	⬇️
src/epsimage.cpp	`1.83% <0.00%> (ø)`
src/exif.cpp	`63.66% <0.00%> (ø)`
src/futils.cpp	`74.84% <ø> (ø)`
src/gifimage.cpp	`38.09% <0.00%> (ø)`
src/orfimage.cpp	`42.68% <0.00%> (ø)`
src/preview.cpp	`47.12% <0.00%> (ø)`
... and 32 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 5d08bb9...fa24fb2. Read the comment docs.

neheb · 2022-03-10T22:44:48Z

Seems fine to me.

clanmills · 2022-03-11T07:47:50Z

A several comments about this.

Purpose

I don't know what problem is being solved here. I've never noticed anything strange about the error handling on 0.27-maintenance. I'm not saying that it's right, nor that I understand it! I'm asking "What's your problem, buddy?". Is it possible to open an issue about error reporting with a solid case where it's causing trouble? If that can be done, does it also apply to 0.27-maintenance?

Localisation1.

I think you should wrap the strings with the N_() macro.

Localisation2.

When you have a message like "My age is " + toString(71) + " you know", it would be better expressed as Internal::stringFormat(N_("My age is %d you know"),71). I think Microsoft have an API that supports formatting directives such as %1. So you could say N_("In the USA my birthday is written %2/%1/%3",18,1,1951)". Don't spend time figuring that out if we don't need it!

clanmills · 2022-03-11T07:54:53Z

@piponazo Yes, sir. You've solved #2116. That's wonderful. Well done.

piponazo · 2022-03-11T08:17:04Z

Hi @clanmills,

The purpose (1) was to solve the warnings you detected on #2116. But not only that issue, Dan and me had this task in our minds for years because there were many issues opened around that templated class over the last years: #1449, #947, #893, #648, #644, etc. Clearly somebody was not right there.

The aim is to keep the functionality as similar as possible to what we had before but simplifying the code. The C++ templating features were wrongly abused around the Error class.

Regarding the localisation stuff (2), where did I miss the usage of the macro N_()? I tried to keep all the strings in errList with those macros as I found them originally. Please, point me to any usage that I might have missed in the PR and I'll fix it.

Finally, regarding (3), I think this would be a task to be addressed in a separated PR. In C++20 we could use the new formatting library which does what you said in a standard way. Something to keep in mind for the future 😸

clanmills · 2022-03-11T12:39:55Z

@piponazo Thanks for the background information, Luis. I remember the discussion that you and Dan had about the templates. @D4N and you understood that and I didn't! Come back @D4N, we miss you.

It's great that you've solved #2116. Sadly, I don't think we can back-port your PR to 0.27-maintenance because it will damage the library API. TBH, I hope we're at the end of 0.27-maintenance. Perhaps a final v0.27.6 about June 2022. I hope to see a plan for 'main' to be released in 2022. C'mon @alexvanderberkel. How about a team meeting?

I don't think we should touch (3) - the %1 %2 formatting stuff. Great to hear that new formatting technology is in the C++20 pipeline. Using stringFormat() would be useful in samples/write-test.cpp However, that's in sample code and not localised and stringFormat()_ is internal. So, maybe nothing to be done. After lunch, I'll go over your PR to see if there are any N_() macro changes. Quite probably there are none.

// *****************************************************************************
void testCase(const std::string& file1,
              const std::string& file2,
              const std::string& thumb,
              const std::string& key,
              const std::string& value)
{
    ExifKey ek(key);

    ...
    if (pos == ed1.end()) {
        throw Error(kerErrorMessage, "Metadatum with key = " + ek.key() + " not found");
    }
    ...
}

clanmills · 2022-03-11T15:03:11Z

@piponazo Can you remove the mention of error-test from README-SAMPLES.md, please.

I've looked at your patch and found the "+" in samples/write-test.cpp

581 rmills@rmillsm1:~/gnu/github/exiv2/mainErrorType $ curl -L https://github.com/Exiv2/exiv2/pull/2141.patch | grep '+' | grep -v ^+ | grep -v ^@ | grep -v '^ samples' | grep -v '^ src' | grep 'Metadatum with key'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   138  100   138    0     0    368      0 --:--:-- --:--:-- --:--:--   372
100  371k    0  371k    0     0   385k      0 --:--:-- --:--:-- --:--:-- 1828k
-        throw Error(kerErrorMessage, "Metadatum with key = " + ek.key() + " not found");
582 rmills@rmillsm1:~/gnu/github/exiv2/mainErrorType $

And looking for the "throws", we find lots that you have fixed!

583 rmills@rmillsm1:~/gnu/github/exiv2/mainErrorType $ curl -L https://github.com/Exiv2/exiv2/pull/2141.patch | grep '+' | grep -v ^+ | grep -v ^@ | grep -v '^ samples' | grep -v '^ src' | grep throw
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   138  100   138    0     0    643      0 --:--:-- --:--:-- --:--:--   654
100  371k    0  371k    0     0   475k      0 --:--:-- --:--:-- --:--:--  475k
-            throw Error(Exiv2::kerFileOpenFailed, f1, "w+b", strError());
-            throw Error(Exiv2::kerFileOpenFailed, "iotest.txt", "w+b", strError());
-            throw Error(Exiv2::kerFileOpenFailed, f2, "w+b", strError());
-        throw Error(kerErrorMessage, "Metadatum with key = " + ek.key() + " not found");
-                throw Error(kerFileOpenFailed, path(), "a+b", strError());
-                throw Error(kerFileOpenFailed, path(), "w+b", strError());
-                throw Error(kerErrorMessage, std::string("Lens regex didn't match for: ") + std::string(lens.label_));
-            throw Error(kerFileOpenFailed, path, "w+b", strError());
-        throw Error(kerErrorMessage, "Invalid native preview filter: " + nativePreview_.filter_);
584 rmills@rmillsm1:~/gnu/github/exiv2/mainErrorType $

I have found 143 "raw/untranslated" strings in the patch. Some such as "Photoshop" don't need attention, some are correct "w+b" and "TIFF" and some are from sample code.

I don't think it's worth spending more time on this. These localisation issues have been in the code forever and nobody has complained.

       throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Key not found");
       throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Downcast failed");
   if (pos == exifData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Key not found");
       throw Exiv2::Error(Exiv2::ErrorCode::kerTiffDirectoryTooLarge, "Server", serverCode);
       throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Unable to init libcurl.");
       throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Timeout Environmental Variable must be a positive integer.");
       throw Exiv2::Error(Exiv2::ErrorCode::kerTiffDirectoryTooLarge, "Server", returnCode);
           throw Error(Exiv2::ErrorCode::kerFileOpenFailed, f1, "w+b", strError());
           throw Error(Exiv2::ErrorCode::kerFileOpenFailed, "iotest.txt", "w+b", strError());
           throw Error(Exiv2::ErrorCode::kerFileOpenFailed, f2, "w+b", strError());
       throw Error(ErrorCode::kerErrorMessage, "Iptc dataset already exists and is not repeatable");
           throw Error(ErrorCode::kerErrorMessage, "Iptc dataset already exists and is not repeatable");
       throw Error(ErrorCode::kerFileOpenFailed, path, "rb", strError());
   if (pos == exifReadData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Exif.Image.Make not found");
   if (pos == exifReadData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Exif.Image.DateTime not found");
   if (pos == exifReadData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Exif.Canon.OwnerName not found");
   if (pos == exifReadData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Exif.CanonCs.LensType not found");
       throw Exiv2::Error(Exiv2::ErrorCode::kerGeneralError, "ARG1", "ARG2", "ARG3");
       throw Error(ErrorCode::kerErrorMessage, "Metadatum with key = " + ek.key() + " not found");
       throw Exiv2::Error(Exiv2::ErrorCode::kerFileOpenFailed, filename, "wb", Exiv2::strError());
       throw Exiv2::Error(Exiv2::ErrorCode::kerCallFailed, filename, Exiv2::strError(), "FileIo::write");
   if (pos == xmpData.end()) throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Key not found");
       throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Failed to serialize XMP data");
           throw Error(ErrorCode::kerCallFailed, path(), strError(), "munmap");
           throw Error(ErrorCode::kerCallFailed, path(), strError(), "mmap");
           throw Error(ErrorCode::kerCallFailed, path(), "MSG1", "_get_osfhandle");
           throw Error(ErrorCode::kerCallFailed, path(), "MSG2", "DuplicateHandle");
           throw Error(ErrorCode::kerCallFailed, path(), "MSG3", "CreateFileMapping");
           throw Error(ErrorCode::kerCallFailed, path(), "MSG4", "CreateFileMapping");
           throw Error(ErrorCode::kerCallFailed, path(), strError(), "FileIo::read");
           throw Error(ErrorCode::kerCallFailed, path(), strError(), "FileIo::mmap");
               throw Error(ErrorCode::kerFileOpenFailed, path(), "a+b", strError());
                           throw Error(ErrorCode::kerCallFailed, pf, strError(), "fs::remove");
                   throw Error(ErrorCode::kerCallFailed, pf, strError(), "fs::remove");
                throw Error(ErrorCode::kerFileOpenFailed, path(), "w+b", strError());
            throw Error(ErrorCode::kerErrorMessage, "No base64 data");
            throw Error(ErrorCode::kerErrorMessage, "Unable to decode base 64.");
                throw Error(ErrorCode::kerErrorMessage, "No base64 data");
                throw Error(ErrorCode::kerErrorMessage, "Unable to decode base 64.");
                throw Error(ErrorCode::kerErrorMessage, "Data By Range is empty. Please check the permission.");
                throw Error(ErrorCode::kerErrorMessage, "the file length is 0");
            throw Error(ErrorCode::kerErrorMessage, "Unable to allocate data");
            throw Error(ErrorCode::kerErrorMessage, "unable to open src when transferring");
            throw Error(ErrorCode::kerFileOpenFailed, "http",Exiv2::Internal::stringFormat("%d",serverCode), hostInfo_.Path);
            throw Error(ErrorCode::kerFileOpenFailed, "http",Exiv2::Internal::stringFormat("%d",serverCode), hostInfo_.Path);
            throw Error(ErrorCode::kerErrorMessage, "Please set the path of the server script to handle http post data to EXIV2_HTTP_POST environmental variable.");
            throw Error(ErrorCode::kerFileOpenFailed, "http",Exiv2::Internal::stringFormat("%d",serverCode), hostInfo_.Path);
            throw Error(ErrorCode::kerErrorMessage, "Unable to init libcurl.");
            throw Error(ErrorCode::kerErrorMessage, "Timeout Environmental Variable must be a positive integer.");
            throw Error(ErrorCode::kerFileOpenFailed, "http",Exiv2::Internal::stringFormat("%d",serverCode),path_);
            throw Error(ErrorCode::kerFileOpenFailed, "http", Exiv2::Internal::stringFormat("%d", serverCode), path_);
            throw Error(ErrorCode::kerErrorMessage, "Please set the path of the server script to handle http post data to EXIV2_HTTP_POST environmental variable.");
            throw Error(ErrorCode::kerFileOpenFailed, "http", Exiv2::Internal::stringFormat("%d", serverCode), path_);
        throw Error(ErrorCode::kerErrorMessage, "doesnt support write for this protocol.");
        throw Error(ErrorCode::kerErrorMessage, "doesnt support write for this protocol.");
            throw Error(ErrorCode::kerFileOpenFailed, path, "rb", strError());
            throw Error(ErrorCode::kerCallFailed, path, strError(), "::stat");
            throw Error(ErrorCode::kerCallFailed, path, strError(), "FileIo::read");
            throw Error(ErrorCode::kerFileOpenFailed, path, "wb", strError());
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "BMFF"));
            throw Error(ErrorCode::kerNotAnImage, "BMFF");
                    throw Exiv2::Error(Exiv2::ErrorCode::kerErrorMessage, "Failed to serialize XMP data");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "BMFF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "BMP"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "BMP"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "BMP"));
            throw Error(ErrorCode::kerNotAnImage, "BMP");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "BMP"));
                throw Error(ErrorCode::kerErrorMessage, std::string("Lens regex didn't match for: ") + std::string(lens.label_));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "CR2"));
            throw Error(ErrorCode::kerNotAnImage, "CR2");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "CRW"));
        throw Error(ErrorCode::kerFunctionNotSupported, "CiffEntry::add");
            throw Error(ErrorCode::kerNotAnImage, "EPS");
        throw Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "EPS");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "GIF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "GIF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "GIF"));
            throw Error(ErrorCode::kerNotAnImage, "GIF");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "GIF"));
            throw Error(ErrorCode::kerFileOpenFailed, path, "w+b", strError());
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "JP2"));
            throw Error(ErrorCode::kerNotAnImage, "JPEG-2000");
                            throw Error(ErrorCode::kerTooLargeJpegSegment, "Exif");
                        throw Error(ErrorCode::kerTooLargeJpegSegment, "XMP");
                        throw Error(ErrorCode::kerTooLargeJpegSegment, "IccProfile");
                        throw Error(ErrorCode::kerTooLargeJpegSegment, "JPEG comment");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "MRW"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "MRW"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "MRW"));
            throw Error(ErrorCode::kerNotAnImage, "MRW");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "MRW"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "ORF"));
            throw Error(ErrorCode::kerNotAnImage, "ORF");
            throw Error(ErrorCode::kerNotAnImage, "PGF");
            throw Error(ErrorCode::kerNotAnImage, "PNG");
            throw Error(ErrorCode::kerNotAnImage, "PNG");
        throw Error(ErrorCode::kerErrorMessage, "Invalid native preview filter: " + nativePreview_.filter_);
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "Photoshop"));
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                    throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
            throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop"); // bad resource type
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                throw Error(ErrorCode::kerNotAnImage, "Photoshop");
                        throw Error(ErrorCode::kerNotAnImage, "Photoshop");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "RAF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "RAF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "RAF"));
            throw Error(ErrorCode::kerNotAnImage, "RAF");
            throw Error(ErrorCode::kerNotAnImage, "RAF");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "RAF"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "RW2"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "RW2"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "RW2"));
            throw Error(ErrorCode::kerNotAnImage, "RW2");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "RW2"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Exif metadata", "TGA"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "TGA"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "TGA"));
            throw Error(ErrorCode::kerNotAnImage, "TGA");
        throw(Error(ErrorCode::kerWritingImageFormatUnsupported, "TGA"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "TIFF"));
            throw Error(ErrorCode::kerNotAnImage, "TIFF");
            throw Error(ErrorCode::kerNotAnImage, "TIFF");
        // throw(Error(ErrorCode::kerInvalidSettingForImage, "IPTC metadata", "WebP"));
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "WebP"));
            throw Error(ErrorCode::kerNotAnImage, "WEBP");
                throw Error(ErrorCode::kerXMPToolkitError, "Could not create expat parser");
                throw Error(ErrorCode::kerXMPToolkitError, "Buffer length is greater than INT_MAX");
        throw Error(ErrorCode::kerFunctionNotSupported, "Xmpdatum::copy");
        throw(Error(ErrorCode::kerInvalidSettingForImage, "Image comment", "XMP"));
            throw Error(ErrorCode::kerNotAnImage, "XMP");

piponazo · 2022-03-11T16:07:07Z

@piponazo Can you remove the mention of error-test from README-SAMPLES.md, please.

Done! :)

+ clang-format in Error files + Remove AnyError

src/error.cpp

piponazo requested review from clanmills, neheb, kevinbackhouse and kmilos March 10, 2022 21:34

piponazo added the refactoring Cleanup / Simplify code -> more readable / robust label Mar 10, 2022

piponazo force-pushed the mainErrorType branch from 41c8990 to e6868b3 Compare March 11, 2022 16:06

piponazo force-pushed the mainErrorType branch from e6868b3 to eff8c38 Compare March 11, 2022 20:18

piponazo added 5 commits March 12, 2022 16:58

Convert Exiv2::ErrorCode into an enum class

96f7f2e

remove templating & simplify error message

7119b76

+ clang-format in Error files + Remove AnyError

Add unit tests for Error class

65568b0

Remove obsolete werror sample

bc7f08b

remove werror-test from README

8d6901c

piponazo force-pushed the mainErrorType branch from eff8c38 to 8d6901c Compare March 12, 2022 16:00

hassec reviewed Mar 13, 2022

View reviewed changes

src/error.cpp Outdated Show resolved Hide resolved

Use compiler deduction guides for error list

fa24fb2

piponazo requested a review from hassec March 14, 2022 09:41

hassec approved these changes Mar 14, 2022

View reviewed changes

piponazo merged commit 173ebb2 into main Mar 14, 2022

piponazo deleted the mainErrorType branch March 14, 2022 09:52

This was referenced Mar 14, 2022

[WIP] Fix the Error type #685

Closed

Ugly/benign linker warnings on macOS / AppleClang #2116

Closed

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

Improvements in the Error class #2141

Improvements in the Error class #2141

piponazo commented Mar 10, 2022

clanmills commented Mar 10, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading

neheb commented Mar 10, 2022

clanmills commented Mar 11, 2022

clanmills commented Mar 11, 2022

piponazo commented Mar 11, 2022

clanmills commented Mar 11, 2022

clanmills commented Mar 11, 2022

piponazo commented Mar 11, 2022

Improvements in the Error class #2141

Improvements in the Error class #2141

Conversation

piponazo commented Mar 10, 2022

clanmills commented Mar 10, 2022

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

neheb commented Mar 10, 2022

clanmills commented Mar 11, 2022

clanmills commented Mar 11, 2022

piponazo commented Mar 11, 2022

clanmills commented Mar 11, 2022

clanmills commented Mar 11, 2022

piponazo commented Mar 11, 2022

codecov bot commented Mar 10, 2022 •

edited

Loading