Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 2 months ago by terelius Modified:
5 years, 1 month ago Reviewers:
ivoc, stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded protobuf message for loss-based BWE events, and wired it up to the send side bandwidth estimator.
BUG=
Committed: https://crrev.com/006d93d3c679ffd15223c327d649066a72400639
Cr-Commit-Position: refs/heads/master@{#10531}
Patch Set 1 : Logging for loss based BWE #
Total comments: 25
Patch Set 2 : Reviewer comments and change sint32 to int32 in protobuf. #Patch Set 3 : Rebase and format #Patch Set 4 : Avoid explicit temporary variable for timestamp. Rebase. #Patch Set 5 : Rebase #
Created: 5 years, 1 month ago
Messages
Total messages: 15 (3 generated)
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org, stefan@webrtc.org
Please have a look at this CL which adds logging capabilities to the loss-based send-side bandwidth estimator.
https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#... webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp); Pass in clock_->TimeInMicroseconds() directly. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n... webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t bitrate, I think we can make this a regular int. Same for total_packets, and maybe even for fraction_loss? https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:108: // required - Bandwidth estimate after the update. unit https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:472: size_t rtcp_index = 1, playout_index = 1, bwe_loss_index = 1; Personally, I prefer to have one variable per line. https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211: event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_, Would there be a point in differentiating between bitrate increases and decreases in the log?
Nice, looks good. Just a few minor nits. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1; I don't really understand why there are 2 signed int's and 1 unsigned int in this message, when as far as I can tell they all represent non-negative numbers. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i < bwe_loss_count; i++) { why not just make i an int? I don't see any compelling reason to make it a size_t (nor in the previous loops for that matter).
https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#... webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp); On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > Pass in clock_->TimeInMicroseconds() directly. I agree with this idea for Rtcp and Rtp packets since they are logged far from the socket, but wouldn't it just be an inconvenience for the caller in the case of BWE events? I mean, the function which updates the birate estimate calls this function directly and this function immediately reads the clock. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n... webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t bitrate, On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > I think we can make this a regular int. Same for total_packets, and maybe even > for fraction_loss? The type here should correspond to the protobuf I think. Since the bitrate is an int32 in the protobuf, there will be an implicit typecast to int32_t when we access the field. The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What we are actually logging is an aggregate of several packets, but the variable still seems to be an uint8_t in all places it is used.) https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:108: // required - Bandwidth estimate after the update. On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > unit Done. (bps) https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1; On 2015/10/19 16:03:38, ivoc wrote: > I don't really understand why there are 2 signed int's and 1 unsigned int in > this message, when as far as I can tell they all represent non-negative numbers. The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The total expected packets is an int in the C++ code. The bitrate in the send side estimator is currently unsigned, but the getters and setters are signed. As negative numbers could potentially be used as some sort of sentinel or special signal, I think it might be good to allow them in the protobuf. Personally I agree with you though; I prefer to use unsigned for all values that should be positive. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i < bwe_loss_count; i++) { On 2015/10/19 16:03:38, ivoc wrote: > why not just make i an int? I don't see any compelling reason to make it a > size_t (nor in the previous loops for that matter). No, there is no strong reason for doing it either way in this case. In general, I prefer to always use size_t for the length of an array and by extension also for indices into an array. The google standard is ambiguous on this issue: "We use int very often, for integers we know are not going to be too big, e.g., loop counters." But also: "When appropriate, you are welcome to use standard types like size_t" Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs 927 occurrences of "for (size_t i". https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:472: size_t rtcp_index = 1, playout_index = 1, bwe_loss_index = 1; On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote: > Personally, I prefer to have one variable per line. Done. https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211: event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_, On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote: > Would there be a point in differentiating between bitrate increases and > decreases in the log? Since we have log the fraction lost, we are able to figure out whether the change should be an increase of decrease. (At least assuming that we know roughly which version of WebRTC is used and the packet loss limits don't change dramatically between WebRTC versions. Current limits are 2% and 10%.) Once we start logging all BWE changes we should also be able to see whether the change was an increase or decrease by comparing whith the previous estimate.
https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#... webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp); On 2015/10/26 17:40:18, terelius wrote: > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > > Pass in clock_->TimeInMicroseconds() directly. > > I agree with this idea for Rtcp and Rtp packets since they are logged far from > the socket, but wouldn't it just be an inconvenience for the caller in the case > of BWE events? I mean, the function which updates the birate estimate calls this > function directly and this function immediately reads the clock. Oh, I meant that instead of doing event.set_timestamp_us(timestamp) you could do: event.set_timestamp_us(clock_->TimeInMicroseconds()); and save one line. I do see that a temporary variable is used in other places here though, so if you prefer that way, go for it. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n... webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t bitrate, On 2015/10/26 17:40:18, terelius wrote: > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > > I think we can make this a regular int. Same for total_packets, and maybe even > > for fraction_loss? > > The type here should correspond to the protobuf I think. Since the bitrate is an > int32 in the protobuf, there will be an implicit typecast to int32_t when we > access the field. > > The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What we > are actually logging is an aggregate of several packets, but the variable still > seems to be an uint8_t in all places it is used.) That is true, although I think it's mostly legacy and there isn't any real reason for using uint8_t or int32_t here. The styleguide suggests using int in most cases except for sizes and in case you actually need a certain size. I don't feel very strongly about this though, so if you think it makes sense to have same types as in the protobuf, then go ahead. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1; On 2015/10/26 17:40:18, terelius wrote: > On 2015/10/19 16:03:38, ivoc wrote: > > I don't really understand why there are 2 signed int's and 1 unsigned int in > > this message, when as far as I can tell they all represent non-negative > numbers. > > The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The total > expected packets is an int in the C++ code. The bitrate in the send side > estimator is currently unsigned, but the getters and setters are signed. As > negative numbers could potentially be used as some sort of sentinel or special > signal, I think it might be good to allow them in the protobuf. > > Personally I agree with you though; I prefer to use unsigned for all values that > should be positive. I'm trying to move most of the BWE code to use ints instead of unsigned types where possible to better follow the styleguide: https://www.chromium.org/developers/coding-style "Do not use unsigned types to mean "this value should never be < 0". For that, use assertions or run-time checks (as appropriate)." That may not apply to protobufs though, actually probably not. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i < bwe_loss_count; i++) { On 2015/10/26 17:40:18, terelius wrote: > On 2015/10/19 16:03:38, ivoc wrote: > > why not just make i an int? I don't see any compelling reason to make it a > > size_t (nor in the previous loops for that matter). > > No, there is no strong reason for doing it either way in this case. In general, > I prefer to always use size_t for the length of an array and by extension also > for indices into an array. > > The google standard is ambiguous on this issue: > "We use int very often, for integers we know are not going to be too big, e.g., > loop counters." > But also: > "When appropriate, you are welcome to use standard types like size_t" > > Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs 927 > occurrences of "for (size_t i". Chromium style is more explicit when it comes to size_t: https://www.chromium.org/developers/coding-style https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... File webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/modules/bitrate_contro... webrtc/modules/bitrate_controller/send_side_bandwidth_estimation.cc:211: event_log_->LogBwePacketLossEvent(bitrate_, last_fraction_loss_, On 2015/10/26 17:40:18, terelius wrote: > On 2015/10/19 08:43:22, stefan-webrtc (holmer) wrote: > > Would there be a point in differentiating between bitrate increases and > > decreases in the log? > > Since we have log the fraction lost, we are able to figure out whether the > change should be an increase of decrease. (At least assuming that we know > roughly which version of WebRTC is used and the packet loss limits don't change > dramatically between WebRTC versions. Current limits are 2% and 10%.) > > Once we start logging all BWE changes we should also be able to see whether the > change was an increase or decrease by comparing whith the previous estimate. True, sgtm
https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc File webrtc/call/rtc_event_log.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.cc#... webrtc/call/rtc_event_log.cc:370: event.set_timestamp_us(timestamp); On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > On 2015/10/26 17:40:18, terelius wrote: > > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > > > Pass in clock_->TimeInMicroseconds() directly. > > > > I agree with this idea for Rtcp and Rtp packets since they are logged far from > > the socket, but wouldn't it just be an inconvenience for the caller in the > case > > of BWE events? I mean, the function which updates the birate estimate calls > this > > function directly and this function immediately reads the clock. > > Oh, I meant that instead of doing event.set_timestamp_us(timestamp) you could > do: > > event.set_timestamp_us(clock_->TimeInMicroseconds()); > > and save one line. I do see that a temporary variable is used in other places > here though, so if you prefer that way, go for it. Done. Sorry, I misunderstood what you wanted. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n... webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t bitrate, On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > On 2015/10/26 17:40:18, terelius wrote: > > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > > > I think we can make this a regular int. Same for total_packets, and maybe > even > > > for fraction_loss? > > > > The type here should correspond to the protobuf I think. Since the bitrate is > an > > int32 in the protobuf, there will be an implicit typecast to int32_t when we > > access the field. > > > > The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What we > > are actually logging is an aggregate of several packets, but the variable > still > > seems to be an uint8_t in all places it is used.) > > That is true, although I think it's mostly legacy and there isn't any real > reason for using uint8_t or int32_t here. The styleguide suggests using int in > most cases except for sizes and in case you actually need a certain size. > > I don't feel very strongly about this though, so if you think it makes sense to > have same types as in the protobuf, then go ahead. It is mostly that I don't think it makes sense to have an unsigned variable, cast it to signed only to immediately cast it back to unsigned to write it in the protobuf. In general, I prefer to make the type casts visible to the user by using the same type in the logging API as in the protobuf. We could conceivably change the protobuf to allow signed ints for the fraction loss though, but since the type is specified in the RFC it seemed fairly safe to assume that it would not change. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1; On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > On 2015/10/26 17:40:18, terelius wrote: > > On 2015/10/19 16:03:38, ivoc wrote: > > > I don't really understand why there are 2 signed int's and 1 unsigned int in > > > this message, when as far as I can tell they all represent non-negative > > numbers. > > > > The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The > total > > expected packets is an int in the C++ code. The bitrate in the send side > > estimator is currently unsigned, but the getters and setters are signed. As > > negative numbers could potentially be used as some sort of sentinel or special > > signal, I think it might be good to allow them in the protobuf. > > > > Personally I agree with you though; I prefer to use unsigned for all values > that > > should be positive. > > I'm trying to move most of the BWE code to use ints instead of unsigned types > where possible to better follow the styleguide: > https://www.chromium.org/developers/coding-style > "Do not use unsigned types to mean "this value should never be < 0". For that, > use assertions or run-time checks (as appropriate)." > > That may not apply to protobufs though, actually probably not. Acknowledged, though I'd like to register my dissatisfaction with the google style guide on this issue. First of all, the style guide example suggests using int for loop counters which contradicts chromiums recommendation of always using size_t. Secondly, the style guide's argument about type conversions between signed and unsigned being dangerous only implies that signed and unsigned should not be mixed. Using only unsigned would also solve that problem. Thirdly, the style guide ignores the fact that the arithmetic for unsigned types is mathematically meaningful whereas arithmetic for signed types is highly error prone and in some cases even semantically undefined. Consider: void f(int high, int low) { assert(high > low); int diff = high - low; assert(diff > 0); // This can fail! } or void f(int x, int y) { int s = x>>10; // This is undefined. (If x is negative) int t = x<<10; // This is actually also undefined. (If x*1024>2^31) } https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:452: for (size_t i = 0; i < bwe_loss_count; i++) { On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > On 2015/10/26 17:40:18, terelius wrote: > > On 2015/10/19 16:03:38, ivoc wrote: > > > why not just make i an int? I don't see any compelling reason to make it a > > > size_t (nor in the previous loops for that matter). > > > > No, there is no strong reason for doing it either way in this case. In > general, > > I prefer to always use size_t for the length of an array and by extension also > > for indices into an array. > > > > The google standard is ambiguous on this issue: > > "We use int very often, for integers we know are not going to be too big, > e.g., > > loop counters." > > But also: > > "When appropriate, you are welcome to use standard types like size_t" > > > > Both are used about equally in WebRTC; 977 occurrences of "for (int i" vs 927 > > occurrences of "for (size_t i". > > Chromium style is more explicit when it comes to size_t: > https://www.chromium.org/developers/coding-style Acknowledged.
lgtm https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h File webrtc/call/rtc_event_log.h (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.h#n... webrtc/call/rtc_event_log.h:77: virtual void LogBwePacketLossEvent(int32_t bitrate, On 2015/10/30 10:51:40, terelius wrote: > On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > > On 2015/10/26 17:40:18, terelius wrote: > > > On 2015/10/19 08:43:21, stefan-webrtc (holmer) wrote: > > > > I think we can make this a regular int. Same for total_packets, and maybe > > even > > > > for fraction_loss? > > > > > > The type here should correspond to the protobuf I think. Since the bitrate > is > > an > > > int32 in the protobuf, there will be an implicit typecast to int32_t when we > > > access the field. > > > > > > The fraction lost field is specified in RFC 3550 as an 8 bit integer. (What > we > > > are actually logging is an aggregate of several packets, but the variable > > still > > > seems to be an uint8_t in all places it is used.) > > > > That is true, although I think it's mostly legacy and there isn't any real > > reason for using uint8_t or int32_t here. The styleguide suggests using int in > > most cases except for sizes and in case you actually need a certain size. > > > > I don't feel very strongly about this though, so if you think it makes sense > to > > have same types as in the protobuf, then go ahead. > > It is mostly that I don't think it makes sense to have an unsigned variable, > cast it to signed only to immediately cast it back to unsigned to write it in > the protobuf. > > In general, I prefer to make the type casts visible to the user by using the > same type in the logging API as in the protobuf. We could conceivably change the > protobuf to allow signed ints for the fraction loss though, but since the type > is specified in the RFC it seemed fairly safe to assume that it would not > change. Agree it doesn't make sense to cast just to cast back later, so let's keep as is then. https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.proto File webrtc/call/rtc_event_log.proto (right): https://codereview.webrtc.org/1411673003/diff/1/webrtc/call/rtc_event_log.pro... webrtc/call/rtc_event_log.proto:109: optional sint32 bitrate = 1; On 2015/10/30 10:51:40, terelius wrote: > On 2015/10/28 16:03:18, stefan-webrtc (holmer) wrote: > > On 2015/10/26 17:40:18, terelius wrote: > > > On 2015/10/19 16:03:38, ivoc wrote: > > > > I don't really understand why there are 2 signed int's and 1 unsigned int > in > > > > this message, when as far as I can tell they all represent non-negative > > > numbers. > > > > > > The fraction lost is defined as an 8-bit unsigned number in RFC 3550. The > > total > > > expected packets is an int in the C++ code. The bitrate in the send side > > > estimator is currently unsigned, but the getters and setters are signed. As > > > negative numbers could potentially be used as some sort of sentinel or > special > > > signal, I think it might be good to allow them in the protobuf. > > > > > > Personally I agree with you though; I prefer to use unsigned for all values > > that > > > should be positive. > > > > I'm trying to move most of the BWE code to use ints instead of unsigned types > > where possible to better follow the styleguide: > > https://www.chromium.org/developers/coding-style > > "Do not use unsigned types to mean "this value should never be < 0". For that, > > use assertions or run-time checks (as appropriate)." > > > > That may not apply to protobufs though, actually probably not. > > Acknowledged, though I'd like to register my dissatisfaction with the google > style guide on this issue. First of all, the style guide example suggests using > int for loop counters which contradicts chromiums recommendation of always using > size_t. Secondly, the style guide's argument about type conversions between > signed and unsigned being dangerous only implies that signed and unsigned should > not be mixed. Using only unsigned would also solve that problem. Thirdly, the > style guide ignores the fact that the arithmetic for unsigned types is > mathematically meaningful whereas arithmetic for signed types is highly error > prone and in some cases even semantically undefined. > > Consider: > void f(int high, int low) { > assert(high > low); > int diff = high - low; > assert(diff > 0); // This can fail! > } > > or > void f(int x, int y) { > int s = x>>10; // This is undefined. (If x is negative) > int t = x<<10; // This is actually also undefined. (If x*1024>2^31) > } Totally agree that there are situations where unsigned makes sense. Especially fixed sizes, such as uint16_t. How can the first example fail?
> > Acknowledged, though I'd like to register my dissatisfaction with the google
> > style guide on this issue. First of all, the style guide example suggests
> using
> > int for loop counters which contradicts chromiums recommendation of always
> using
> > size_t. Secondly, the style guide's argument about type conversions between
> > signed and unsigned being dangerous only implies that signed and unsigned
> should
> > not be mixed. Using only unsigned would also solve that problem. Thirdly,
the
> > style guide ignores the fact that the arithmetic for unsigned types is
> > mathematically meaningful whereas arithmetic for signed types is highly
error
> > prone and in some cases even semantically undefined.
> >
> > Consider:
> > void f(int high, int low) {
> > assert(high > low);
> > int diff = high - low;
> > assert(diff > 0); // This can fail!
> > }
> >
> > or
> > void f(int x, int y) {
> > int s = x>>10; // This is undefined. (If x is negative)
> > int t = x<<10; // This is actually also undefined. (If x*1024>2^31)
> > }
>
> Totally agree that there are situations where unsigned makes sense. Especially
> fixed sizes, such as uint16_t.
>
> How can the first example fail?
Because the (positive) difference between two ints might be larger than what is
representable as an int. We actually had an instance of this bug in the random
number generator recently. Basically a scaling factor could become negative
which caused our uniform random numbers to be neither uniform, nor in the
requested range.
LGTM.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1411673003/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411673003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411673003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/006d93d3c679ffd15223c327d649066a72400639 Cr-Commit-Position: refs/heads/master@{#10531} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
