Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696003004:
Added integer parsing functions in base/string_to_number.h (Closed)
|
Created:
3 years, 10 months ago by ossu Modified:
3 years, 8 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded integer parsing functions in base/string_to_number.h
They provide parsing of numbers from char* and std::string into any of
the fundamental integer types. Values are returned
in an rtc::Optional, which is left empty if parsing was unsuccessful.
BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2696003004
Cr-Commit-Position: refs/heads/master@{#17557}
Committed: https://chromium.googlesource.com/external/webrtc/+/a280f7c02647e80052ce344703a1557f52e924da
Patch Set 1 #Patch Set 2 : Added more debug printing to invalid inputs tests. #
Total comments: 24
Patch Set 3 : Removed float support, moved template instances into .cc, made parsing stricter. #
Total comments: 6
Patch Set 4 : Separated implementation from templates. Made parsing stricter. Added a couple of specific tests. #
Total comments: 13
Patch Set 5 : long long int -> long long and some clang-formatting. #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Added work-around for (unsigned) long long int being banned by lint. #
Total comments: 28
Patch Set 9 : Addressed comments. #
Dependent Patchsets: Messages
Total messages: 53 (34 generated)
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Added number parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer and floating-point types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG= ========== to ========== Added number parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer and floating-point types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG= ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13520)
Hey Karl, PTAL at this CL as a work-in-progress - both at design and implementation. I considered having separate names for parsing integers and floats, but decided against it since it made it more of a hassle to use in templates - specifically the parameterized tests. It's maybe a bit strange that the integer versions take a second parameter for overriding the base of the number and the floating-point versions don't. Practically, it's probably not much of an issue. I'm planning to add a few more tests: one set for testing integer parsing at various bases and one set for testing stuff that's special for floats, like NaN and +/-infinity.
Patchset #2 (id:20001) has been deleted
I like the approach. 1. Have you made sure that we can't just steal similar functions from Chromium? Parsing numbers isn't a particularly exotic problem... 2. You let a bunch of questionable features from std::strto* bleed through: ignoring leading spaces, ignoring trailing garbage, locale dependent definitions of things like the decimal point, and probably more stuff that I've missed. Unless you actually need this, I would suggest you make a much stricter validity check before passing the string to std::strto* https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:47: StringToNumber(const char* str, int base = 10) { You probably want to static_assert that long long is at least as large as your type. (Hmm. Is that guaranteed by the standard? Even if it is, it's probably good to assert it to give readers peace of mind...) https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:73: } You allow multiple minus signs---that's probably not a good idea. (In fact, I'd argue that it may make sense to reject "-0" and similar when the result type is unsigned, but I don't feel strongly about it.) https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:120: } You could probably unify these three if you made a templated wrapper for strtof/strtod/strtold. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:134: } Could you ditch this one if you defaulted base to 10 on line 125? https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:23: typedef ::testing::Types<signed char, unsigned char, using? https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:42: const auto lowest_value = std::numeric_limits<TypeParam>::lowest(); "min_value"? I don't think the lowest/min naming of numeric_limits is a good model to emulate... https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:55: EXPECT_EQ(0, StringToNumber<TypeParam>(std::string("-0000000000000"))); Hmm. Consider "using T = TypeParam" in these methods, to reduce verbosity. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:56: } Consider testing a few specific simple cases, such as 0, 1, -1, 17, etc. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:66: // approximately ten times larger. If the type is unsigned, just use -2. Prepending a "1" doesn't make a value approximately 10 times larger---consider e.g. "9" -> "19". You could say "at least twice as large", or just append "0" instead and make the comment true. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:75: << *StringToNumber<TypeParam>(too_low_string); Do you need to repeat the parsed number? Won't EXPECT_EQ print it already?
On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > I like the approach. > > 1. Have you made sure that we can't just steal similar functions from Chromium? > Parsing numbers isn't a particularly exotic problem... It didn't occur to me. I'll look into it! > 2. You let a bunch of questionable features from std::strto* bleed through: > ignoring leading spaces, ignoring trailing garbage, locale dependent definitions > of things like the decimal point, and probably more stuff that I've missed. > Unless you actually need this, I would suggest you make a much stricter validity > check before passing the string to std::strto* Hmm. Of those, my biggest concern would be the decimal point - the others I can accept from a purely practical standpoint. The decimal point, however, would uncontrollably break parsing, and must be fixed. I'll look into it. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:47: StringToNumber(const char* str, int base = 10) { On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > You probably want to static_assert that long long is at least as large as your > type. (Hmm. Is that guaranteed by the standard? Even if it is, it's probably > good to assert it to give readers peace of mind...) Makes sense. The tests should catch any case where that is not true already, though. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:73: } On 2017/02/16 00:27:23, kwiberg-webrtc wrote: > You allow multiple minus signs---that's probably not a good idea. (In fact, I'd > argue that it may make sense to reject "-0" and similar when the result type is > unsigned, but I don't feel strongly about it.) I should check, but I'd expect stroull would reject it if it were invalid. My check for - is only to reject slightly more things. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:120: } On 2017/02/16 00:27:23, kwiberg-webrtc wrote: > You could probably unify these three if you made a templated wrapper for > strtof/strtod/strtold. Yeah, that's true. I'll look into that. As-is, the implementations need to be moved into a .cc-file, since fully specialized templates act like normal functions; I got multiple definitions of the same symbol when linking in another CL I'm working on that uses this. I'm considering putting all implementations in a .cc file, and explicitly instantiating them for the basic types. So we don't risk ending up inlining this everywhere it's used. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:134: } On 2017/02/16 00:27:23, kwiberg-webrtc wrote: > Could you ditch this one if you defaulted base to 10 on line 125? Alas, no, there's no base parameter to the floating-point variants. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:23: typedef ::testing::Types<signed char, unsigned char, On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > using? Sure! https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:42: const auto lowest_value = std::numeric_limits<TypeParam>::lowest(); On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > "min_value"? I don't think the lowest/min naming of numeric_limits is a good > model to emulate... I think it would be more confusing if min meant two different things within the same function, i.e. the definition from numeric_limits and the "regular" one. If this were non-test code, I'd consider it more important to conform to the rest of our code than to numeric_limits. I'll consider it. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:55: EXPECT_EQ(0, StringToNumber<TypeParam>(std::string("-0000000000000"))); On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > Hmm. Consider "using T = TypeParam" in these methods, to reduce verbosity. Sure! https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:66: // approximately ten times larger. If the type is unsigned, just use -2. On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > Prepending a "1" doesn't make a value approximately 10 times larger---consider > e.g. "9" -> "19". You could say "at least twice as large", or just append "0" > instead and make the comment true. That is true. Initially I appended a "1" instead, but realized this might not work well with floats: 1.00000011 is also not ten times larger than 1.0000001 and 1.00000010 is not larger at all. I'll have to re-think this a bit as well, since some implementations (see failed tests) put in +/-inf for these values, and 1inf is just garbage. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:75: << *StringToNumber<TypeParam>(too_low_string); On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > Do you need to repeat the parsed number? Won't EXPECT_EQ print it already? No. It can't print out things through rtc::Optional. I think we should have a look at fixing that, since we're making more use of Optional and it's making our tests results unreadable. Unfortunately, there doesn't seem to be an operator<< for std::optional. gtest also supports printing an object if there's a PrintTo(const Obj&, ostream*) function declared, so maybe we can use that?
https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:47: StringToNumber(const char* str, int base = 10) { On 2017/02/16 10:19:26, ossu wrote: > On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > > You probably want to static_assert that long long is at least as large as your > > type. (Hmm. Is that guaranteed by the standard? Even if it is, it's probably > > good to assert it to give readers peace of mind...) > > Makes sense. The tests should catch any case where that is not true already, > though. Yes, but it's easier for a human to see that the static_assert tests actually catch everything. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:73: } On 2017/02/16 10:19:26, ossu wrote: > On 2017/02/16 00:27:23, kwiberg-webrtc wrote: > > You allow multiple minus signs---that's probably not a good idea. (In fact, > I'd > > argue that it may make sense to reject "-0" and similar when the result type > is > > unsigned, but I don't feel strongly about it.) > > I should check, but I'd expect stroull would reject it if it were invalid. My > check for - is only to reject slightly more things. Oh, I see. I thought you were discarding the minuses, but you still pass them to std::strtoull. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number.h:120: } On 2017/02/16 10:19:26, ossu wrote: > On 2017/02/16 00:27:23, kwiberg-webrtc wrote: > > You could probably unify these three if you made a templated wrapper for > > strtof/strtod/strtold. > > Yeah, that's true. I'll look into that. As-is, the implementations need to be > moved into a .cc-file, since fully specialized templates act like normal > functions; I got multiple definitions of the same symbol when linking in another > CL I'm working on that uses this. > > I'm considering putting all implementations in a .cc file, and explicitly > instantiating them for the basic types. So we don't risk ending up inlining this > everywhere it's used. Sounds good. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:66: // approximately ten times larger. If the type is unsigned, just use -2. On 2017/02/16 10:19:26, ossu wrote: > On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > > Prepending a "1" doesn't make a value approximately 10 times larger---consider > > e.g. "9" -> "19". You could say "at least twice as large", or just append "0" > > instead and make the comment true. > > That is true. Initially I appended a "1" instead, but realized this might not > work well with floats: 1.00000011 is also not ten times larger than 1.0000001 > and 1.00000010 is not larger at all. I'll have to re-think this a bit as well, > since some implementations (see failed tests) put in +/-inf for these values, > and 1inf is just garbage. Oh right, I totally forgot about the floating point case. https://codereview.webrtc.org/2696003004/diff/40001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:75: << *StringToNumber<TypeParam>(too_low_string); On 2017/02/16 10:19:26, ossu wrote: > On 2017/02/16 00:27:24, kwiberg-webrtc wrote: > > Do you need to repeat the parsed number? Won't EXPECT_EQ print it already? > > No. It can't print out things through rtc::Optional. I think we should have a > look at fixing that, since we're making more use of Optional and it's making our > tests results unreadable. Unfortunately, there doesn't seem to be an operator<< > for std::optional. gtest also supports printing an object if there's a > PrintTo(const Obj&, ostream*) function declared, so maybe we can use that? Yes, that sounds good.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13631)
I've removed support for floats, since I don't need them and they're too much of a hassle until I do. I've also moved the implementations into the .cc file and explicitly instantiate them for all basic types from char up to long long int. Having them explicitly instantiated like that might cause problems if there are types that don't convert exactly to one of those types - like if some system had a uint16_t that didn't actually match any of those types. I cannot preemptively instantiate them for the intX_t types as well, since if one resolves to one of the standard types, it will get instantiated twice. I'm thinking of putting the (unsigned) long long variants in the .cc file and have a tiny template that just adds the extra min/max check in the header. That should resolve the issue. Finally, I've tightened up the parsing a bit. If the end pointer from stroll doesn't point to a 0, the result is rejected. I might add rejection for strings starting with spaces as well, though they are not very risky, imo.
https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... File webrtc/base/string_to_number.cc (right): https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number.cc:20: sizeof(long long int) >= sizeof(T), Compare std::numeric_limits<>::min() and ::max() instead? Since the types have the same signedness, the comparison should work just fine. https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number.cc:26: if (end && *end == 0 && end != str && errno == 0 && Style nit: *end == '\0' https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number.cc:57: value >= std::numeric_limits<T>::min() && value and T are both unsigned, so this is trivially true. https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number.cc:59: return rtc::Optional<T>(static_cast<T>(value)); Hmm. Do you have a unit test for e.g. parsing "-256" to a uint8_t? https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:43: const auto max_value = std::numeric_limits<T>::max(); constexpr T https://codereview.webrtc.org/2696003004/diff/80001/webrtc/base/string_to_num... webrtc/base/string_to_number_unittest.cc:59: // largest type, which is another hassle. Could you make one or a few tests for some specific types, to test that sort of thing? The [u]intN_t types should be convenient...
On 2017/02/17 15:18:11, ossu wrote: > I've removed support for floats, since I don't need them and they're too much of > a hassle until I do. > > I've also moved the implementations into the .cc file and explicitly instantiate > them for all basic types from char up to long long int. Having them explicitly > instantiated like that might cause problems if there are types that don't > convert exactly to one of those types - like if some system had a uint16_t that > didn't actually match any of those types. I cannot preemptively instantiate them > for the intX_t types as well, since if one resolves to one of the standard > types, it will get instantiated twice. > > I'm thinking of putting the (unsigned) long long variants in the .cc file and > have a tiny template that just adds the extra min/max check in the header. That > should resolve the issue. That sounds like a good idea---having internal non-template functions for [unsigned] long long, and then public inline template wrappers in the .h file that just call the internal functions and do some static_asserts and casts. The current implementation generates code for a dozen extremely similar functions. > Finally, I've tightened up the parsing a bit. If the end pointer from stroll > doesn't point to a 0, the result is rejected. I might add rejection for strings > starting with spaces as well, though they are not very risky, imo. Personally, I'd reject numbers with leading space garbage too, but I'm fine with either decision.
I've made the implementation regular functions, with smaller templates in the header to do conversion and range checking. Also added a few specific tests for values. Not really sure which others I should add, if any. On 2017/02/18 12:33:10, kwiberg-webrtc wrote: > Personally, I'd reject numbers with leading space garbage too, but I'm fine with > either decision. I think you're right. Might as well start out strict, so users won't start relying on the lax behavior.
lgtm nits are all optional https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:44: namespace string_to_number_internal { Bonus points for the very specific internal namespace name! https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:45: rtc::Optional<long long int> ParseSigned(const char* str, int base); Just "long long" means the same thing as "long long int", right? And it's not quite as long... https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:58: "long long int"); Multi-line string literals are a bit easier to read if you have leading instead of trailing spaces (since the left but not the right margin tends to be aligned). https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:62: *value <= std::numeric_limits<T>::max()) { If you pass the min and max value to the internal parse function, you can do this check there---that'll mean less code to inline everywhere. Not sure if it's worth it, though; you decide. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:63: return rtc::Optional<T>(static_cast<T>(*value)); This ought to be dchecked_cast, but I won't request that you write that before landing this CL... :-)
https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:44: namespace string_to_number_internal { On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > Bonus points for the very specific internal namespace name! Thanks! :) https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:45: rtc::Optional<long long int> ParseSigned(const char* str, int base); On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > Just "long long" means the same thing as "long long int", right? And it's not > quite as long... Done. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:58: "long long int"); On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > Multi-line string literals are a bit easier to read if you have leading instead > of trailing spaces (since the left but not the right margin tends to be > aligned). I'm not sure I agree and clang-format seems to split strings like this. I've rejiggered them a bit, though, so they should be a bit easier to read. They also fit better with the shorter type names. I chose not to change the type name in the message, since I find it made the message flow strangely. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:62: *value <= std::numeric_limits<T>::max()) { On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > If you pass the min and max value to the internal parse function, you can do > this check there---that'll mean less code to inline everywhere. Not sure if it's > worth it, though; you decide. I think this is clearer - we'll need to do a cast anyway, so it's nice to ensure the range is good in the same place. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:63: return rtc::Optional<T>(static_cast<T>(*value)); On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > This ought to be dchecked_cast, but I won't request that you write that before > landing this CL... :-) I didn't find that one. Found a checked_cast, though, but it seems to do what I'm already doing on the previous line, so I don't think it's necessary.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13762)
Description was changed from ========== Added number parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer and floating-point types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG= ========== to ========== Added integer parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG=webrtc:5806 ==========
https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:58: "long long int"); On 2017/02/20 14:09:06, ossu wrote: > On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > > Multi-line string literals are a bit easier to read if you have leading > instead > > of trailing spaces (since the left but not the right margin tends to be > > aligned). > > I'm not sure I agree and clang-format seems to split strings like this. I've > rejiggered them a bit, though, so they should be a bit easier to read. They also > fit better with the shorter type names. I chose not to change the type name in > the message, since I find it made the message flow strangely. Acknowledged. Not everyone can be right all the time. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:62: *value <= std::numeric_limits<T>::max()) { On 2017/02/20 14:09:06, ossu wrote: > On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > > If you pass the min and max value to the internal parse function, you can do > > this check there---that'll mean less code to inline everywhere. Not sure if > it's > > worth it, though; you decide. > > I think this is clearer - we'll need to do a cast anyway, so it's nice to ensure > the range is good in the same place. Acknowledged. https://codereview.webrtc.org/2696003004/diff/100001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:63: return rtc::Optional<T>(static_cast<T>(*value)); On 2017/02/20 14:09:06, ossu wrote: > On 2017/02/20 13:27:20, kwiberg-webrtc wrote: > > This ought to be dchecked_cast, but I won't request that you write that before > > landing this CL... :-) > > I didn't find that one. Found a checked_cast, though, but it seems to do what > I'm already doing on the previous line, so I don't think it's necessary. Yes, we only have checked_cast. dchecked_cast doesn't exist yet, but should...
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15754)
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Preparing to start landing all the stuff depending on this. Had to do some minor changes to make presubmit happy: the normal int types, like long long int, were not allowed. I'd argue they're exactly the ones needed here, to match the signature of strto(u)ll, so I've done a workaround using an alias and NOLINT. Without the aliases, almost all the lines would have to be NOLINT. :) PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:21: const signed_type value = std::strtoll(str, &end, base); #include <cstdlib>? https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:22: if (end && *end == '\0' && end != str && errno == 0) { That end != str is necessarily true if the first two conditions hold. DCHECK instead? https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:38: const unsigned_type value = std::strtoull(str, &end, base); Wouldn't it be safer to discard the minus sign before giving the string to strtoull? https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:45: // These must be (unsigned) long long, to match the signature of strto(u)ll. I suppose you *could* use [u]intmax_t, and just static_assert that their ranges are at least as large as that of [unsigned] long long (they're supposed to be), but it seems much simpler to use the actual correct types like you do here, and suppressing the lint warnings. (Why does lint not like long long, by the way?) https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:50: rtc::Optional<unsigned_type> ParseUnsigned(const char* str, int base); Consider letting these take min and max arguments. That way, the logic that tests if the parsed number is in range of the type could move out of the inlined templated functions. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:66: if (value && *value >= std::numeric_limits<T>::min() && ::lowest()? Doesn't matter for integers, but nice to be consistent. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:68: return rtc::Optional<T>(static_cast<T>(*value)); dchecked_cast? It exists now! https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:86: return rtc::Optional<T>(static_cast<T>(*value)); dchecked_cast? https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:24: ::testing::Types<signed char, unsigned char, // NOLINT(runtime/int) And char! It isn't the same as either signed or unsigned char... https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:80: const rtc::Optional<T> kEmptyOptional; Unused. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:93: EXPECT_EQ(rtc::Optional<T>(), StringToNumber<T>(" -5")); What happens if we try to parse a number followed by space, e.g. "5 "?
https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:21: const signed_type value = std::strtoll(str, &end, base); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > #include <cstdlib>? Ach, yes. Not sure why I didn't get IWYU warnings for that. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:22: if (end && *end == '\0' && end != str && errno == 0) { On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > That end != str is necessarily true if the first two conditions hold. DCHECK > instead? I'm guessing maybe if it would highlight an error by letting end be str and str was the empty string. The docs I'm reading say end points to "the character past the last character interpreted". Eh, I don't know. Should probably be NULL if conversion fails. I'll try without and see if the tests are still happy. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:38: const unsigned_type value = std::strtoull(str, &end, base); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > Wouldn't it be safer to discard the minus sign before giving the string to > strtoull? Nah, it should be able to deal with that itself, according to the docs. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:45: // These must be (unsigned) long long, to match the signature of strto(u)ll. On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > I suppose you *could* use [u]intmax_t, and just static_assert that their ranges > are at least as large as that of [unsigned] long long (they're supposed to be), > but it seems much simpler to use the actual correct types like you do here, and > suppressing the lint warnings. > > (Why does lint not like long long, by the way?) Lint doesn't like any of the non stdint types, except for int and char, I think. Essentially, if you want to size your integers, it wants you to use the types with explicit sizes, to avoid errors. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:50: rtc::Optional<unsigned_type> ParseUnsigned(const char* str, int base); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > Consider letting these take min and max arguments. That way, the logic that > tests if the parsed number is in range of the type could move out of the inlined > templated functions. The templated function would still do the type conversion, so I think it's clearer to have the check there. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:66: if (value && *value >= std::numeric_limits<T>::min() && On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > ::lowest()? Doesn't matter for integers, but nice to be consistent. Will do! https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:68: return rtc::Optional<T>(static_cast<T>(*value)); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > dchecked_cast? It exists now! I've _just_ checked the ranges. I'm not going to do it again! https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:86: return rtc::Optional<T>(static_cast<T>(*value)); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > dchecked_cast? See above. :) https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:24: ::testing::Types<signed char, unsigned char, // NOLINT(runtime/int) On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > And char! It isn't the same as either signed or unsigned char... Really? Well I'll be darned... https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:80: const rtc::Optional<T> kEmptyOptional; On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > Unused. Acknowledged. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:93: EXPECT_EQ(rtc::Optional<T>(), StringToNumber<T>(" -5")); On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > What happens if we try to parse a number followed by space, e.g. "5 "? I can add a test for that if you want. Should be the same as a number followed by any other character: failure.
https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:22: if (end && *end == '\0' && end != str && errno == 0) { On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > That end != str is necessarily true if the first two conditions hold. DCHECK > > instead? > > I'm guessing maybe if it would highlight an error by letting end be str and str > was the empty string. The docs I'm reading say end points to "the character past > the last character interpreted". Eh, I don't know. Should probably be NULL if > conversion fails. I'll try without and see if the tests are still happy. It'll still work: since *end == '\0' and (isdigit(str[0]) || str[0] == '-'), you know that end != str. So you can either move it to a DCHECK, or just remove it. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.cc:38: const unsigned_type value = std::strtoull(str, &end, base); On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > Wouldn't it be safer to discard the minus sign before giving the string to > > strtoull? > > Nah, it should be able to deal with that itself, according to the docs. Acknowledged. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number.h (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:45: // These must be (unsigned) long long, to match the signature of strto(u)ll. On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > I suppose you *could* use [u]intmax_t, and just static_assert that their > ranges > > are at least as large as that of [unsigned] long long (they're supposed to > be), > > but it seems much simpler to use the actual correct types like you do here, > and > > suppressing the lint warnings. > > > > (Why does lint not like long long, by the way?) > > Lint doesn't like any of the non stdint types, except for int and char, I think. > Essentially, if you want to size your integers, it wants you to use the types > with explicit sizes, to avoid errors. Acknowledged. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:50: rtc::Optional<unsigned_type> ParseUnsigned(const char* str, int base); On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > Consider letting these take min and max arguments. That way, the logic that > > tests if the parsed number is in range of the type could move out of the > inlined > > templated functions. > > The templated function would still do the type conversion, so I think it's > clearer to have the check there. But more code to inline. But fair enough, either way works. https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number.h:68: return rtc::Optional<T>(static_cast<T>(*value)); On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > dchecked_cast? It exists now! > > I've _just_ checked the ranges. I'm not going to do it again! Well, that's what dchecked_cast means: "I promise that this cast will not fail to preserve the value in the new type". It won't actually check that you're telling the truth (except in debug builds). https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... File webrtc/base/string_to_number_unittest.cc (right): https://codereview.webrtc.org/2696003004/diff/180001/webrtc/base/string_to_nu... webrtc/base/string_to_number_unittest.cc:93: EXPECT_EQ(rtc::Optional<T>(), StringToNumber<T>(" -5")); On 2017/04/05 10:15:36, ossu wrote: > On 2017/04/05 09:01:15, kwiberg-webrtc wrote: > > What happens if we try to parse a number followed by space, e.g. "5 "? > > I can add a test for that if you want. Should be the same as a number followed > by any other character: failure. Good. But I figured a reasonable conversion function might treat spaces differently from other junk, which is why a test might make sense.
Patchset #9 (id:200001) has been deleted
Patchset #9 (id:220001) has been deleted
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
I've addressed your comments, except for the cast one, since I still don't see what it would add. All good to go?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491469189570780,
"parent_rev": "b1e3fc4515a4d3cfb0078357bf133e90c15db9a2", "commit_rev":
"a280f7c02647e80052ce344703a1557f52e924da"}
Message was sent while issue was closed.
Description was changed from ========== Added integer parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG=webrtc:5806 ========== to ========== Added integer parsing functions in base/string_to_number.h They provide parsing of numbers from char* and std::string into any of the fundamental integer types. Values are returned in an rtc::Optional, which is left empty if parsing was unsuccessful. BUG=webrtc:5806 Review-Url: https://codereview.webrtc.org/2696003004 Cr-Commit-Position: refs/heads/master@{#17557} Committed: https://chromium.googlesource.com/external/webrtc/+/a280f7c02647e80052ce34470... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001) as https://chromium.googlesource.com/external/webrtc/+/a280f7c02647e80052ce34470... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
