[WIP] Fix the Error type #685
Conversation
Codecov Report
@@ Coverage Diff @@
## master #685 +/- ##
==========================================
+ Coverage 71.18% 71.19% +<.01%
==========================================
Files 148 149 +1
Lines 19370 19373 +3
==========================================
+ Hits 13789 13793 +4
+ Misses 5581 5580 -1
Continue to review full report at Codecov.
|
|
Thanks for dealing with this, Dan. Grunt work. I'll leave @piponazo to approve the change. Can you apply this to both 0.27 and master? |
Not for 0.27, this PR requires C++11. On top of that, this is a quite intrusive change API & ABI wise, so pushing this to 0.27 is not a good idea imho. |
|
I took a look to the changes and I like the simplification of things 👍 . I also think this should only reach master. It is also a big change, and there is always a risk of breaking something without noticing ... Let me know when you are done with the changes to take another look . |
|
Please do not merge this! This PR must not be merged before we have enabled unicode paths on Windows & MinGW. |
D4N
force-pushed
the
fix_error_type
branch
3 times, most recently
from
0439119
to
9da5027
Compare
|
I'm setting the target to 0.28 and the code from this PR should NOT be considered for porting to v0.27 as this would inflict substantial changes to the 0.27 API. We shouldn't consider this matter related to #947 nor #644. I'm going to search for a solution to #947 and #646 in 0.27-maintenance and I don't expect the fix to be needed in 0.28. I'll be happy to work with @D4N to resolve issues on Windows to remove the template declarations in error.hpp. |
|
Ey @D4N , I see that know the builds are passing in all the CI builders. What's the plan about this PR ? |
| @@ -481,47 +482,47 @@ namespace Exiv2 { | |||
| if (hFd == INVALID_HANDLE_VALUE) { | |||
| #ifdef EXV_UNICODE_PATH | |||
| if (p_->wpMode_ == Impl::wpUnicode) { | |||
| throw WError(kerCallFailed, wpath(), "MSG1", "_get_osfhandle"); | |||
| throw Error(ErrorCode::kerCallFailed, ws2s(wpath()), "MSG1", "_get_osfhandle"); | |||
There was a problem hiding this comment.
In basicio.cpp I really dislike all the code duplication we have to throw an exception with either wpath() or path(). I would remove the path argument from this call so that we can remove lot of code from this file.
In my opinion is not very useful to incluse the file path in the exception, since we normally use the exiv2 command line tool just with one file (and therefore we know which file is being used) if that exception is thrown.
There was a problem hiding this comment.
I don't like this either, although I'm mostly annoyed by the need to explicitly call ws2s(foo). That should be possible to remove, but I didn't have the time to code something up yet.
The following:
|
|
@D4N The conflicts are massive, especially basicio.cpp, after haveing thi PR open for more than one year. Any chances to progress? |
|
I am closing this PR since we already merged the proposed changes in #2141 |
This is another huge PR, but it essentially does only a single thing: remove the templating of
BasicError<charT>and replace this class withError, which is no longer generic. I used this opportunity to convert theErrorCodesenum to an enumeration class, deprecateAnyError(which is now completely pointless) and optimize the creation of the error messages a little bit (there is still room for improvement though).This PR is not yet ready, it should be merged into master after #617 is forward ported and we have at least some form of MinGW CI set up (see #645 and #663). Also we have to enable unicode paths on Appveyor before merging this.