Add cpp-httplib local modification to correct sign comparison error. [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +1 |
| Commit-Queue | +1 |
Related details
- Joshua Peraza
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
You should share the error here.
You should also say what steps you’ve taken to upstream this, or if you haven’t upstreamed, why not.
const int num_objs = static_cast<int>(sk_X509_OBJECT_num(objs));Can you do this without the cast at all? Assignments should be subject to more lax rules than comparisons.
for (int i = 0; i < sk_X509_OBJECT_num(objs); i++) {This is already an `int` in the headers I’ve looked at. What SSL library is in use when this breaks? (This is why you should show the error, and include a link to where you saw it.)
Related details
- Joshua Peraza
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 3 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +0 |
| Commit-Queue | +1 |
You should share the error here.
You should also say what steps you’ve taken to upstream this, or if you haven’t upstreamed, why not.
There appear to be a lot of local modifications, so while there's only one call to sk_X509_OBJECT_num in our repo, there are 4 upstream. I wanted to at least get through this review.
I'll update the description with the error details.
const int num_objs = static_cast<int>(sk_X509_OBJECT_num(objs));Can you do this without the cast at all? Assignments should be subject to more lax rules than comparisons.
Yes, `const int num_objs = sk_X509_OBJECT_num(objs);` works, I'll update to that.
for (int i = 0; i < sk_X509_OBJECT_num(objs); i++) {This is already an `int` in the headers I’ve looked at. What SSL library is in use when this breaks? (This is why you should show the error, and include a link to where you saw it.)
There seems to be a bunch of macros at work in BoringSSL causing this. Compiling Chromium with -E I see:
inline size_t sk_X509_OBJECT_num(const struct stack_st_X509_OBJECT *sk) { return OPENSSL_sk_num((const OPENSSL_STACK *)sk); }
I tried to make a minified sample using httplib and boringssl but couldn't get the same setup as Chromium.
Related details
- Joshua Peraza
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Justin CohenYou should share the error here.
You should also say what steps you’ve taken to upstream this, or if you haven’t upstreamed, why not.
There appear to be a lot of local modifications, so while there's only one call to sk_X509_OBJECT_num in our repo, there are 4 upstream. I wanted to at least get through this review.
I'll update the description with the error details.
PR here: https://github.com/yhirose/cpp-httplib/pull/2355, noted in CL description
Related details
- Joshua Peraza
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
In some build configurations, sk_X509_OBJECT_num (from BoringSSL)Oh, does BoringSSL also use `size_t` as `sk_X509_OBJECT_VALUE`’s parameter’s type?
If so, this would be a good time to deploy `auto` and `decltype`, or use other modern C++ magic to get at the type you need.
```
auto count = sk_X509_OBJECT_num(objs);
for (decltype(count) i = 0; i < count; i++) {
```
I took your `const` away because you don’t want `i` to be `const`. Too bad, I liked the `const` on `count`, actually. You could put it back (`auto const count = …`, or order the `const` in the more traditional but decidedly barbaric way) and remove it from `i` with `std::remove_const_t<decltype(count)>` from `<type_traits>`, or `std::remove_const<decltype(count)>::type` if you need pre-C++14 compatibility, which hopefully you do not.
This should be totally compatible with any TLS library with an OpenSSL-like interface (including both OpenSSL and BoringSSL) as long as it uses the a stack count and index type consistently.
This preserves BoringSSL’s (superior) interface enhancements when using BoringSSL, while still remaining fully compatible with classic OpenSSL.
If you don’t like `decltype`, the other magic I have in mind that you can substitute is `std::invoke_result_t` if C++17 is assured, but I like that less because it’s library (`<type_traits>`), not language (but this is concededly a weak argument if you’re already using `std::remove_const_t`); it had a different name before C++17 if that’s important; and it’s wordier without any real clear gain.
Related details
- Joshua Peraza
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
In some build configurations, sk_X509_OBJECT_num (from BoringSSL)Oh, does BoringSSL also use `size_t` as `sk_X509_OBJECT_VALUE`’s parameter’s type?
If so, this would be a good time to deploy `auto` and `decltype`, or use other modern C++ magic to get at the type you need.
```
auto count = sk_X509_OBJECT_num(objs);
for (decltype(count) i = 0; i < count; i++) {
```I took your `const` away because you don’t want `i` to be `const`. Too bad, I liked the `const` on `count`, actually. You could put it back (`auto const count = …`, or order the `const` in the more traditional but decidedly barbaric way) and remove it from `i` with `std::remove_const_t<decltype(count)>` from `<type_traits>`, or `std::remove_const<decltype(count)>::type` if you need pre-C++14 compatibility, which hopefully you do not.
This should be totally compatible with any TLS library with an OpenSSL-like interface (including both OpenSSL and BoringSSL) as long as it uses the a stack count and index type consistently.
This preserves BoringSSL’s (superior) interface enhancements when using BoringSSL, while still remaining fully compatible with classic OpenSSL.
If you don’t like `decltype`, the other magic I have in mind that you can substitute is `std::invoke_result_t` if C++17 is assured, but I like that less because it’s library (`<type_traits>`), not language (but this is concededly a weak argument if you’re already using `std::remove_const_t`); it had a different name before C++17 if that’s important; and it’s wordier without any real clear gain.
- Joshua Peraza
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
3 comments
Add cpp-httplib local modification to correct sign comparison error.I think “BoringSSL compatibility” is more important and belongs here in the summary line. You should still talk about -Wsign-compare below, but there are only so many things you can fit in the first line.
- Correct sign compare error in sk_X509_OBJECT_num.Say that this is for BoringSSL compatibility.
Related details
- Joshua Peraza
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +0 |
1 comment
Phone gnubby didn't work.
Related details
- Joshua Peraza
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
LGTM with the requested documentary changes.
(Trying gnubby from non-phone. Wish me luck!)
Related details
- Joshua Peraza
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |