Skip to content
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

Enhance: Use unordered_map for CURL error mapping #1142

Merged
merged 9 commits into from
Nov 19, 2024

Conversation

laxerhd
Copy link
Contributor

@laxerhd laxerhd commented Nov 13, 2024

Here is the corresponding PR which should close #1140, regarding some enhancements to the error handling in cpr.
I replaced the switch-case statement used for CURL error mapping with an unordered_map.

When using clang-tidy I am warned that the initialization of 'curl_error_map' with static storage duration may throw an exception that cannot be caught.

I am unsure how we want to solve this:

  • I could resolve this by introducing lazy initialization, wrapping the unordered_map inside a function that guarantees safe runtime initialization (but would probably make it a little bit harder to read)
  • or, just ignore the message with // NOLINT (I don't like that)

The first solution would probably look something like this:

// Using this function instead of the curl_error_map variable
const std::unordered_map<std::int32_t, ErrorCode>& getCurlErrorMap() {
    static const std::unordered_map<std::int32_t, ErrorCode> curl_error_map = {
        {CURLE_OK, ErrorCode::OK},
        {CURLE_UNSUPPORTED_PROTOCOL, ErrorCode::UNSUPPORTED_PROTOCOL},
        {CURLE_FAILED_INIT, ErrorCode::FAILED_INIT},
        ...,
        };
    return curl_error_map;
}        

and then call this function in the Error::getErrorCodeForCurlError.

Are there any other options you would suggest?

Also, I wasn't sure for which error codes we include version checking (and how to find out when they were released), but I could find some mentions of PARTIAL_FILE in some changelogs in early 2003, so I didn't add any checks for that.

@laxerhd
Copy link
Contributor Author

laxerhd commented Nov 13, 2024

Like I already said, I am not quite sure how to proceed with the clang-tidy warning.

@COM8
Copy link
Member

COM8 commented Nov 14, 2024

Like I already said, I am not quite sure how to proceed with the clang-tidy warning.

This is due to an updated version of clang-tidy. I fixed it in this PR: #1144

Once merged, you can rebase onto the latest master.

Regarding initialization of 'curl_error_map' with static storage duration may throw an exception that cannot be caught. Please // NOLINT it since it's a C lib and there is nothing we can do against it. Also the ctr of it is so simple that we can ignore it.

@laxerhd laxerhd marked this pull request as ready for review November 16, 2024 15:49
Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Looks good! Please reabase onto: #1144

@laxerhd
Copy link
Contributor Author

laxerhd commented Nov 17, 2024

I hope I rebased it the right way... ;P

Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rebasing, but something went wrong there. You now also include the commits from my MR. This should not be the case.

How did you rebase?
The normal workflow for this would be:

  • Add the original repo (this one) as e.g. forkOrigin.
  • Rebase onto forkOrigin/master.
  • git push --force-with-lease

PS: Top check which symbols are available in which curl version: https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions

cpr/error.cpp Outdated
Comment on lines 2 to 3
#include <cstdint>
#include <curl/curl.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you are missing those two includes for clang-tidy to work.

Copy link
Contributor Author

@laxerhd laxerhd Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I removed them because there was no apparant reason for them.
I will add them back, but why does clang-tidy need them?

@laxerhd laxerhd force-pushed the enhance/error_handling branch from f761d6c to 330725b Compare November 17, 2024 17:32
@laxerhd laxerhd force-pushed the enhance/error_handling branch from 330725b to 8b0ccd2 Compare November 17, 2024 17:38
@laxerhd
Copy link
Contributor Author

laxerhd commented Nov 17, 2024

Did the cleanup.
Your commits should not be included anymore...

Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM!

@COM8 COM8 merged commit 9d9908c into libcpr:master Nov 19, 2024
50 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving Error Handling: Missing PARTIAL_FILE and long switch-case refactoring
2 participants