Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by Taylor Brandstetter Modified:
3 years, 10 months ago Reviewers:
tommi, kjellander_webrtc, kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, kwiberg-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding RTCErrorOr class to be used by ORTC APIs.
This utility class can be used to represent either an error or a
successful return value. Follows the pattern of StatusOr in the protobuf
library.
This will be used by ORTC factory methods; for instance, CreateRtpSender
will either return an RtpSender or an error if the parameters are
invalid or some other failure occurs.
This CL also moves RTCError classes to a separate file, and adds tests
that were missing before.
BUG=webrtc:7013
Review-Url: https://codereview.webrtc.org/2692723002
Cr-Commit-Position: refs/heads/master@{#16659}
Committed: https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce0d84d0798c65f8b
Patch Set 1 #
Total comments: 28
Patch Set 2 : Updates based on tommi's comments #
Total comments: 9
Patch Set 3 : Use const char* for error message. #
Total comments: 5
Patch Set 4 : Cleaning up how union works. #Patch Set 5 : Removing using statements. #Patch Set 6 : Adding test and changing default RTCErrorOr to INTERNAL_ERROR #Patch Set 7 : Ading "MoveError". Found a use for it in CL in progress. #
Total comments: 2
Patch Set 8 : Adding build dependency. #Patch Set 9 : Changing "CreateAndLogError" to a macro. #
Total comments: 4
Created: 3 years, 10 months ago
Dependent Patchsets: Messages
Total messages: 47 (14 generated)
deadbeef@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi: Peter thought it would be a good idea for you to look at this. Here's an example of how it will be used: https://codereview.webrtc.org/2675173003/diff/230001/webrtc/api/ortc/ortcfact... https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode94 webrtc/api/rtcerror.h:94: static const RTCError& OK(); This is the only thing other than RTCErrorOr and CreateAndLogError that's new. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode123 webrtc/api/rtcerror.h:123: rtc::LoggingSeverity severity); Can be used like: return CreateAndLogError(RTCErrorType::INVALID_PARAMETER, "Foo must be between 1 and 10.");
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; have you checked if this is thread safe? I don't think it is on Windows at least. I think there is some singleton code elsewhere that you could take a look at. Alterternatively, if it is guaranteed that this method is always called on the same thread or if it is always called once on a known thread before any other thread can call it, it can be considered thread safe and it would be good to document that here. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode87 webrtc/api/rtcerror.h:87: RTCError(RTCErrorType type, std::string message) is |message| expected to be std::move()d? also, use std::move in the initializer list? https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode103 webrtc/api/rtcerror.h:103: std::string message() const { return message_; } return const&? https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode117 webrtc/api/rtcerror.h:117: std::ostream& operator<<(std::ostream& stream, RTCErrorType error); Streams are costly in terms of code size and cpu usage. Is this only for for debug logging and test diagnostics? If not, I prefer we figure out a way to do this that does not involve iostream. We don't need all the bloat that comes with it such as localization. Some more information here: https://google.github.io/styleguide/cppguide.html#Streams https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode184 webrtc/api/rtcerror.h:184: RTCErrorOr(const RTCError& error) : error_(error) { RTC_DCHECK(!error.ok()); } could this be |RTCError&& error| and move ownership to RTCErrorOr? Since RTCError is a complex object, it would be good if the API is designed to avoid creating copies whenever we use RTCErrorOr as the documentation suggests. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode195 webrtc/api/rtcerror.h:195: // Copy constructor. do we need one? (comment?) if we could instead live with being only movable, that would be nice. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229 webrtc/api/rtcerror.h:229: T& value() { can you add a comment that explains when this is needed?
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/12 12:11:21, tommi (webrtc) wrote: > have you checked if this is thread safe? I don't think it is on Windows at > least. > I think there is some singleton code elsewhere that you could take a look at. > Alterternatively, if it is guaranteed that this method is always called on the > same thread or if it is always called once on a known thread before any other > thread can call it, it can be considered thread safe and it would be good to > document that here. It's thread-safe according to the standard, though Microsoft didn't implement this until VS2015. Does webrtc have a minimum version of VS it targets? chromium builds with VS2015. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode87 webrtc/api/rtcerror.h:87: RTCError(RTCErrorType type, std::string message) On 2017/02/12 12:11:22, tommi (webrtc) wrote: > is |message| expected to be std::move()d? also, use std::move in the > initializer list? Just meant to use a const ref, fixed. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode103 webrtc/api/rtcerror.h:103: std::string message() const { return message_; } On 2017/02/12 12:11:21, tommi (webrtc) wrote: > return const&? Done. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode117 webrtc/api/rtcerror.h:117: std::ostream& operator<<(std::ostream& stream, RTCErrorType error); On 2017/02/12 12:11:22, tommi (webrtc) wrote: > Streams are costly in terms of code size and cpu usage. Is this only for for > debug logging and test diagnostics? If not, I prefer we figure out a way to do > this that does not involve iostream. We don't need all the bloat that comes with > it such as localization. > > Some more information here: > https://google.github.io/styleguide/cppguide.html#Streams Yes, it's only intended for logging. I'll add a comment. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode184 webrtc/api/rtcerror.h:184: RTCErrorOr(const RTCError& error) : error_(error) { RTC_DCHECK(!error.ok()); } On 2017/02/12 12:11:21, tommi (webrtc) wrote: > could this be |RTCError&& error| and move ownership to RTCErrorOr? Since > RTCError is a complex object, it would be good if the API is designed to avoid > creating copies whenever we use RTCErrorOr as the documentation suggests. Done. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode195 webrtc/api/rtcerror.h:195: // Copy constructor. On 2017/02/12 12:11:21, tommi (webrtc) wrote: > do we need one? (comment?) > if we could instead live with being only movable, that would be nice. I can't think of where it would be useful. I'll remove it for now. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229 webrtc/api/rtcerror.h:229: T& value() { On 2017/02/12 12:11:21, tommi (webrtc) wrote: > can you add a comment that explains when this is needed? It could be useful if the implementation, say, wants to check something about the value of an RTCErrorOr before passing it further up the stack. RTCErrorOr<Foo> MakeFooInternal(Params p) { // stuff return Foo(p); } RTCErrorOr<Foo> MakeFoo(Params p) { auto result = MakeFooInternal(p); if (result.ok()) { LOG(LS_INFO) << "Created Foo with ID: " << result.value().id(); } return result; } Bad example, since that logging could be done in MakeFooInternal, but that's the idea.
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229 webrtc/api/rtcerror.h:229: T& value() { On 2017/02/12 16:19:23, Taylor Brandstetter wrote: > On 2017/02/12 12:11:21, tommi (webrtc) wrote: > > can you add a comment that explains when this is needed? > > It could be useful if the implementation, say, wants to check something about > the value of an RTCErrorOr before passing it further up the stack. > > RTCErrorOr<Foo> MakeFooInternal(Params p) { > // stuff > return Foo(p); > } > > RTCErrorOr<Foo> MakeFoo(Params p) { > auto result = MakeFooInternal(p); > if (result.ok()) { > LOG(LS_INFO) << "Created Foo with ID: " << result.value().id(); > } > return result; > } > > Bad example, since that logging could be done in MakeFooInternal, but that's the > idea. Would the |const T& value()| accessor cover that case? I'm specifically wondering about the ability to be able to change |value| outside of the RTCErrorOr class.
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229 webrtc/api/rtcerror.h:229: T& value() { On 2017/02/12 16:27:28, tommi (webrtc) wrote: > On 2017/02/12 16:19:23, Taylor Brandstetter wrote: > > On 2017/02/12 12:11:21, tommi (webrtc) wrote: > > > can you add a comment that explains when this is needed? > > > > It could be useful if the implementation, say, wants to check something about > > the value of an RTCErrorOr before passing it further up the stack. > > > > RTCErrorOr<Foo> MakeFooInternal(Params p) { > > // stuff > > return Foo(p); > > } > > > > RTCErrorOr<Foo> MakeFoo(Params p) { > > auto result = MakeFooInternal(p); > > if (result.ok()) { > > LOG(LS_INFO) << "Created Foo with ID: " << result.value().id(); > > } > > return result; > > } > > > > Bad example, since that logging could be done in MakeFooInternal, but that's > the > > idea. > > Would the |const T& value()| accessor cover that case? > I'm specifically wondering about the ability to be able to change |value| > outside of the RTCErrorOr class. There are still some cases where the non-const-ref accessor is convenient. RTCErrorOr<Foo> MakeGeneralFoo(...); RTCErrorOr<Foo> MakeVideoFoo(...) { auto result = MakeGeneralFoo(...); if (!result.ok()) { return result; } result.value().DoVideoSpecificThing(); return result; } RTCErrorOr<Foo> MakeAudioFoo(...) { // ... }
I'm heading out, so just sending the two drafts I had. Will take a look at the latest patch later https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/12 16:19:23, Taylor Brandstetter wrote: > On 2017/02/12 12:11:21, tommi (webrtc) wrote: > > have you checked if this is thread safe? I don't think it is on Windows at > > least. > > I think there is some singleton code elsewhere that you could take a look at. > > Alterternatively, if it is guaranteed that this method is always called on the > > same thread or if it is always called once on a known thread before any other > > thread can call it, it can be considered thread safe and it would be good to > > document that here. > > It's thread-safe according to the standard, though Microsoft didn't implement > this until VS2015. Does webrtc have a minimum version of VS it targets? chromium > builds with VS2015. Looks like the change in chromium was made a couple of weeks ago: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static... We're following that so this should be fine. Since it's such a recent change though, I would like to confirm with kjellander@ before landing. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode229 webrtc/api/rtcerror.h:229: T& value() { On 2017/02/12 16:44:12, Taylor Brandstetter wrote: > On 2017/02/12 16:27:28, tommi (webrtc) wrote: > > On 2017/02/12 16:19:23, Taylor Brandstetter wrote: > > > On 2017/02/12 12:11:21, tommi (webrtc) wrote: > > > > can you add a comment that explains when this is needed? > > > > > > It could be useful if the implementation, say, wants to check something > about > > > the value of an RTCErrorOr before passing it further up the stack. > > > > > > RTCErrorOr<Foo> MakeFooInternal(Params p) { > > > // stuff > > > return Foo(p); > > > } > > > > > > RTCErrorOr<Foo> MakeFoo(Params p) { > > > auto result = MakeFooInternal(p); > > > if (result.ok()) { > > > LOG(LS_INFO) << "Created Foo with ID: " << result.value().id(); > > > } > > > return result; > > > } > > > > > > Bad example, since that logging could be done in MakeFooInternal, but that's > > the > > > idea. > > > > Would the |const T& value()| accessor cover that case? > > I'm specifically wondering about the ability to be able to change |value| > > outside of the RTCErrorOr class. > > There are still some cases where the non-const-ref accessor is convenient. > > RTCErrorOr<Foo> MakeGeneralFoo(...); > > RTCErrorOr<Foo> MakeVideoFoo(...) { > auto result = MakeGeneralFoo(...); > if (!result.ok()) { > return result; > } > result.value().DoVideoSpecificThing(); > return result; > } > > RTCErrorOr<Foo> MakeAudioFoo(...) { > // ... > } OK, if it's necessary, I'm not against it. What I was thinking was that if we don't actually need it now, it's easier to add it later if we need it than remove it after usage gets added without us noticing (e.g. if a method should have been made const but wasn't made so since things compile without it).
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/12 18:19:32, tommi (webrtc) wrote: > On 2017/02/12 16:19:23, Taylor Brandstetter wrote: > > On 2017/02/12 12:11:21, tommi (webrtc) wrote: > > > have you checked if this is thread safe? I don't think it is on Windows at > > > least. > > > I think there is some singleton code elsewhere that you could take a look > at. > > > Alterternatively, if it is guaranteed that this method is always called on > the > > > same thread or if it is always called once on a known thread before any > other > > > thread can call it, it can be considered thread safe and it would be good to > > > document that here. > > > > It's thread-safe according to the standard, though Microsoft didn't implement > > this until VS2015. Does webrtc have a minimum version of VS it targets? > chromium > > builds with VS2015. > > Looks like the change in chromium was made a couple of weeks ago: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static... > > We're following that so this should be fine. Since it's such a recent change > though, I would like to confirm with kjellander@ before landing. I'd rather avoid singletons as much as possible. It doesn't scale well; i.e. bad for backend scenario.
https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:112: std::string message_; Can we avoid allocating a string for every error? Won't some be either empty or a static string? https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:251: RTCError error_; this also means that every return value that uses RTCErrorOr, creates a string, which seems unnecessary.
Description was changed from ========== Adding RTCErrorOr class to be used by ORTC APIs. This utility class can be used to represent either an error or a successful return value. Follows the pattern of StatusOr in the protobuf library. This will be used by ORTC factory methods; for instance, CreateRtpSender will either return an RtpSender or an error if the parameters are invalid or some other failure occurs. This CL also moves RTCError classes to a separate file, and adds tests that were missing before. BUG=webrtc:7013 ========== to ========== Adding RTCErrorOr class to be used by ORTC APIs. This utility class can be used to represent either an error or a successful return value. Follows the pattern of StatusOr in the protobuf library. This will be used by ORTC factory methods; for instance, CreateRtpSender will either return an RtpSender or an error if the parameters are invalid or some other failure occurs. This CL also moves RTCError classes to a separate file, and adds tests that were missing before. BUG=webrtc:7013 ==========
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/13 07:35:03, the sun wrote: > > I'd rather avoid singletons as much as possible. It doesn't scale well; i.e. bad > for backend scenario. I'm curious what the downside here is. But would you prefer it returning a newly constructed RTCError? https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:112: std::string message_; On 2017/02/13 12:10:01, tommi (webrtc) wrote: > Can we avoid allocating a string for every error? Won't some be either empty or > a static string? No, there are cases where I want to build the string. https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:251: RTCError error_; On 2017/02/13 12:10:01, tommi (webrtc) wrote: > this also means that every return value that uses RTCErrorOr, creates a string, > which seems unnecessary. I'd hoped that, by using move constructors/assignment, most the overhead of strings would be reduced. But I don't expect "RTCError"s to be used in any code that's a performance bottleneck, since this is intended for just the API layer.
https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:104: void set_message(const std::string& message) { message_ = message; } is this method necessary? It could have benefits to require that the message only be set at construction time. The message would be const, doesn't have to be a complex object like a std::string or even set at all. https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:112: std::string message_; On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > On 2017/02/13 12:10:01, tommi (webrtc) wrote: > > Can we avoid allocating a string for every error? Won't some be either empty > or > > a static string? > > No, there are cases where I want to build the string. I think the answer to "Won't some be either empty or a static string?" will still be Yes. If that's the case, the interface to reading the stream (the message() method), could return const char*. That would allow the implementation to return a static string pointer or a std::string::c_str() when a more complex string has to be built. https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:251: RTCError error_; On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > On 2017/02/13 12:10:01, tommi (webrtc) wrote: > > this also means that every return value that uses RTCErrorOr, creates a > string, > > which seems unnecessary. > > I'd hoped that, by using move constructors/assignment, most the overhead of > strings would be reduced. > > But I don't expect "RTCError"s to be used in any code that's a performance > bottleneck, since this is intended for just the API layer. I understand. The overhead can still be reduced further and if this class will be a part of the public API, I think it's worth considering if we're over promising in the interface in a way that prevents the implementation from being optimized.
https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:104: void set_message(const std::string& message) { message_ = message; } On 2017/02/13 18:17:14, tommi (webrtc) wrote: > is this method necessary? It could have benefits to require that the message > only be set at construction time. The message would be const, doesn't have to be > a complex object like a std::string or even set at all. The PeerConnection methods that take an "RTCError*" as an out parameter for backwards compatibility need it. That may be the only reason. https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:112: std::string message_; On 2017/02/13 18:17:14, tommi (webrtc) wrote: > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > On 2017/02/13 12:10:01, tommi (webrtc) wrote: > > > Can we avoid allocating a string for every error? Won't some be either > empty > > or > > > a static string? > > > > No, there are cases where I want to build the string. > > I think the answer to "Won't some be either empty or a static string?" will > still be Yes. > If that's the case, the interface to reading the stream (the message() method), > could return const char*. That would allow the implementation to return a static > string pointer or a std::string::c_str() when a more complex string has to be > built. Oh, I didn't see "some", sorry. Hmm. But if "message" returns a "const char*", then the code above webrtc will still have to convert it to a string again which will result in a copy, unless it uses "const char*" everywhere the message is passed along.
On 2017/02/13 18:44:18, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h > File webrtc/api/rtcerror.h (right): > > https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... > webrtc/api/rtcerror.h:104: void set_message(const std::string& message) { > message_ = message; } > On 2017/02/13 18:17:14, tommi (webrtc) wrote: > > is this method necessary? It could have benefits to require that the message > > only be set at construction time. The message would be const, doesn't have to > be > > a complex object like a std::string or even set at all. > > The PeerConnection methods that take an "RTCError*" as an out parameter for > backwards compatibility need it. That may be the only reason. > > https://codereview.webrtc.org/2692723002/diff/20001/webrtc/api/rtcerror.h#new... > webrtc/api/rtcerror.h:112: std::string message_; > On 2017/02/13 18:17:14, tommi (webrtc) wrote: > > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > > On 2017/02/13 12:10:01, tommi (webrtc) wrote: > > > > Can we avoid allocating a string for every error? Won't some be either > > empty > > > or > > > > a static string? > > > > > > No, there are cases where I want to build the string. > > > > I think the answer to "Won't some be either empty or a static string?" will > > still be Yes. > > If that's the case, the interface to reading the stream (the message() > method), > > could return const char*. That would allow the implementation to return a > static > > string pointer or a std::string::c_str() when a more complex string has to be > > built. > > Oh, I didn't see "some", sorry. > > Hmm. But if "message" returns a "const char*", then the code above webrtc will > still have to convert it to a string again which will result in a copy, unless > it uses "const char*" everywhere the message is passed along. Well, I was thinking if we had something like: union { const char* static_; std::string* str_; } message_; then we could return either message_.static_ or message.str_->c_str() depending on which was needed. In Chromium, a lot of the error strings are just static strings and all that needs to be passed around, is a pointer. In rare cases, a string needs to be constructed, so that's also supported, but there has been a lot of work done to back out of designs that require string copies, so I'm thinking that here, we can optimize for (presumably) the most common case (no error), support the second most common case in an efficient case (static error string) and still support the rich string case, which I'm assuming will be the rarest. sorry for being difficult with this btw :) I'm trying to make up for it by replying quickly.
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > On 2017/02/13 07:35:03, the sun wrote: > > > > I'd rather avoid singletons as much as possible. It doesn't scale well; i.e. > bad > > for backend scenario. > > I'm curious what the downside here is. But would you prefer it returning a newly > constructed RTCError? Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be "static const"?
On 2017/02/13 19:04:06, tommi (webrtc) wrote:
>
> Well, I was thinking if we had something like:
>
> union {
> const char* static_;
> std::string* str_;
> } message_;
>
> then we could return either message_.static_ or message.str_->c_str()
depending
> on which was needed.
> In Chromium, a lot of the error strings are just static strings and all that
> needs to be passed around, is a pointer. In rare cases, a string needs to be
> constructed, so that's also supported, but there has been a lot of work done
to
> back out of designs that require string copies, so I'm thinking that here, we
> can optimize for (presumably) the most common case (no error), support the
> second most common case in an efficient case (static error string) and still
> support the rich string case, which I'm assuming will be the rarest.
>
> sorry for being difficult with this btw :) I'm trying to make up for it by
> replying quickly.
Ok, if this is the approach chromium is taking, you convinced me. See updated
patchset.
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/13 19:28:52, the sun wrote: > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > On 2017/02/13 07:35:03, the sun wrote: > > > > > > I'd rather avoid singletons as much as possible. It doesn't scale well; i.e. > > bad > > > for backend scenario. > > > > I'm curious what the downside here is. But would you prefer it returning a > newly > > constructed RTCError? > > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be > "static const"? Done. By the way, when I did this, I got a compile warning that was only fixed by changing the constructor from "RTCError() = default" to "RTCError() {}". Would you happen to know why? Aren't these doing the same thing?
On 2017/02/13 22:24:15, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc > File webrtc/api/rtcerror.cc (right): > > https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 > webrtc/api/rtcerror.cc:41: static RTCError ok; > On 2017/02/13 19:28:52, the sun wrote: > > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > > On 2017/02/13 07:35:03, the sun wrote: > > > > > > > > I'd rather avoid singletons as much as possible. It doesn't scale well; > i.e. > > > bad > > > > for backend scenario. > > > > > > I'm curious what the downside here is. But would you prefer it returning a > > newly > > > constructed RTCError? > > > > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be > > "static const"? > > Done. By the way, when I did this, I got a compile warning that was only fixed > by changing the constructor from "RTCError() = default" to "RTCError() {}". > Would you happen to know why? Aren't these doing the same thing? That's interesting! No, I don't know. If anything I'd have guessed "= default" would possess more magic powers. :) Do you have any idea kwiberg@?
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/13 22:24:15, Taylor Brandstetter wrote: > On 2017/02/13 19:28:52, the sun wrote: > > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > > On 2017/02/13 07:35:03, the sun wrote: > > > > > > > > I'd rather avoid singletons as much as possible. It doesn't scale well; > i.e. > > > bad > > > > for backend scenario. > > > > > > I'm curious what the downside here is. But would you prefer it returning a > > newly > > > constructed RTCError? > > > > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it be > > "static const"? > > Done. By the way, when I did this, I got a compile warning that was only fixed > by changing the constructor from "RTCError() = default" to "RTCError() {}". > Would you happen to know why? Aren't these doing the same thing? Interesting. What did the message say? This page (http://en.cppreference.com/w/cpp/language/default_constructor) has a section "Deleted implicitly-declared default constructor" on circumstances under which the implicitly-declared default constructor is deleted. I can't see that any of them would apply here, though. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode243 webrtc/api/rtcerror.h:243: T value_; Hmm. So an RTCErrorOr always contains a T. This may be inefficient, and may be problematic if T isn't default-constructable. https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:88: // the message is a static string. If this sort of thing is a consideration, you may want to take the std::string by value, so that the caller can std::move it. Or add a third constructor that takes a std::string&&.
https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:88: // the message is a static string. On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > If this sort of thing is a consideration, you may want to take the std::string > by value, so that the caller can std::move it. Or add a third constructor that > takes a std::string&&. this is a consideration because there's no need to allocate memory + copy for a static string
https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:88: // the message is a static string. On 2017/02/14 09:55:56, tommi (webrtc) wrote: > On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > > If this sort of thing is a consideration, you may want to take the std::string > > by value, so that the caller can std::move it. Or add a third constructor that > > takes a std::string&&. > > this is a consideration because there's no need to allocate memory + copy for a > static string Then I'd suggest having *only* the const char* and std::string&& variants, so that callers are forced to either pass a static string or pass an rvalue std::string. That makes it hard for them to make performance-degrading mistakes.
https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:88: // the message is a static string. On 2017/02/14 11:37:25, kwiberg-webrtc wrote: > On 2017/02/14 09:55:56, tommi (webrtc) wrote: > > On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > > > If this sort of thing is a consideration, you may want to take the > std::string > > > by value, so that the caller can std::move it. Or add a third constructor > that > > > takes a std::string&&. > > > > this is a consideration because there's no need to allocate memory + copy for > a > > static string > > Then I'd suggest having *only* the const char* and std::string&& variants, so > that callers are forced to either pass a static string or pass an rvalue > std::string. That makes it hard for them to make performance-degrading mistakes. That sounds good to me. I may have mentioned it a couple of patch sets ago, maybe there's some reason that Taylor knows about.
Another weird thing I ran into: this doesn't work: str_.~string(); But this does: str_.~basic_string(); It sounds like this is a bug? http://stackoverflow.com/questions/24593942/how-to-explicitly-call-a-namespac... https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.cc#newcode41 webrtc/api/rtcerror.cc:41: static RTCError ok; On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > On 2017/02/13 22:24:15, Taylor Brandstetter wrote: > > On 2017/02/13 19:28:52, the sun wrote: > > > On 2017/02/13 18:03:53, Taylor Brandstetter wrote: > > > > On 2017/02/13 07:35:03, the sun wrote: > > > > > > > > > > I'd rather avoid singletons as much as possible. It doesn't scale well; > > i.e. > > > > bad > > > > > for backend scenario. > > > > > > > > I'm curious what the downside here is. But would you prefer it returning a > > > newly > > > > constructed RTCError? > > > > > > Sorry, I misread the code in the morning haze. Looks fine, but shouldn't it > be > > > "static const"? > > > > Done. By the way, when I did this, I got a compile warning that was only fixed > > by changing the constructor from "RTCError() = default" to "RTCError() {}". > > Would you happen to know why? Aren't these doing the same thing? > > Interesting. What did the message say? > > This page (http://en.cppreference.com/w/cpp/language/default_constructor) has a > section "Deleted implicitly-declared default constructor" on circumstances under > which the implicitly-declared default constructor is deleted. I can't see that > any of them would apply here, though. "default initialization of an object of const type 'const webrtc::RTCError' without a user-provided default constructor". It seems it's the union it's unhappy with. Maybe the union default constructor doesn't work how I thought it would, or the compiler has a bug. I changed the union anyway though. https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/1/webrtc/api/rtcerror.h#newcode243 webrtc/api/rtcerror.h:243: T value_; On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > Hmm. So an RTCErrorOr always contains a T. This may be inefficient, and may be > problematic if T isn't default-constructable. I plan to use this for things that are either small or unique_ptrs, so I think this should be fine. https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h File webrtc/api/rtcerror.h (right): https://codereview.webrtc.org/2692723002/diff/40001/webrtc/api/rtcerror.h#new... webrtc/api/rtcerror.h:88: // the message is a static string. On 2017/02/14 11:58:58, tommi (webrtc) wrote: > On 2017/02/14 11:37:25, kwiberg-webrtc wrote: > > On 2017/02/14 09:55:56, tommi (webrtc) wrote: > > > On 2017/02/14 09:51:49, kwiberg-webrtc wrote: > > > > If this sort of thing is a consideration, you may want to take the > > std::string > > > > by value, so that the caller can std::move it. Or add a third constructor > > that > > > > takes a std::string&&. > > > > > > this is a consideration because there's no need to allocate memory + copy > for > > a > > > static string > > > > Then I'd suggest having *only* the const char* and std::string&& variants, so > > that callers are forced to either pass a static string or pass an rvalue > > std::string. That makes it hard for them to make performance-degrading > mistakes. > > That sounds good to me. I may have mentioned it a couple of patch sets ago, > maybe there's some reason that Taylor knows about. Done.
Thanks for jumping through all the hoops Taylor. lgtm assuming the destructor strangeness isn't a problem in practice. Might be good to document it though. https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n... webrtc/api/rtcerror.cc:62: string_message_.~basic_string(); interesting - did you hit this problem with all tool chains?
The CQ bit was checked by tommi@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: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17350) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22785) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21553) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18787) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22802)
https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc File webrtc/api/rtcerror.cc (right): https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n... webrtc/api/rtcerror.cc:62: string_message_.~basic_string(); On 2017/02/15 16:27:07, tommi (webrtc) wrote: > interesting - did you hit this problem with all tool chains? I'd only tried with gcc. This might be expected behavior though, since string is a typedef.
On 2017/02/15 21:14:21, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc > File webrtc/api/rtcerror.cc (right): > > https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n... > webrtc/api/rtcerror.cc:62: string_message_.~basic_string(); > On 2017/02/15 16:27:07, tommi (webrtc) wrote: > > interesting - did you hit this problem with all tool chains? > > I'd only tried with gcc. This might be expected behavior though, since string is > a typedef. Ah yeah, could be. Wonder if you could do the "technically" correct thing by using a template? something like template<typename T> void CallDestructor(T* t) { t->~T(); }
On 2017/02/15 21:32:46, tommi (webrtc) wrote: > On 2017/02/15 21:14:21, Taylor Brandstetter wrote: > > https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc > > File webrtc/api/rtcerror.cc (right): > > > > > https://codereview.webrtc.org/2692723002/diff/120001/webrtc/api/rtcerror.cc#n... > > webrtc/api/rtcerror.cc:62: string_message_.~basic_string(); > > On 2017/02/15 16:27:07, tommi (webrtc) wrote: > > > interesting - did you hit this problem with all tool chains? > > > > I'd only tried with gcc. This might be expected behavior though, since string > is > > a typedef. > > Ah yeah, could be. Wonder if you could do the "technically" correct thing by > using a template? something like > template<typename T> > void CallDestructor(T* t) { t->~T(); } bah, nevermind, if basic_string works, that's probably more readable and at least as correct :)
On 2017/02/15 21:32:46, tommi (webrtc) wrote:
> Wonder if you could do the "technically" correct thing by
> using a template? something like
> template<typename T>
> void CallDestructor(T* t) { t->~T(); }
Oh, that must be why rtc::Optional<std::string> works. Templates do so much
magic I don't understand.
I changed "CreateAndLogError" to a macro, for a couple reasons: 1. It doesn't work with chromium's "logging.h" override. 2. It doesn't log the line of code that called it, which makes it much less useful.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2692723002/#ps160001 (title: "Changing "CreateAndLogError" to a macro.")
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": 160001, "attempt_start_ts": 1487313299614570,
"parent_rev": "74520c6718fc8ebc6da9fec1ce28b1b49112a921", "commit_rev":
"6038e97e048bebf763f7149ce0d84d0798c65f8b"}
Message was sent while issue was closed.
Description was changed from ========== Adding RTCErrorOr class to be used by ORTC APIs. This utility class can be used to represent either an error or a successful return value. Follows the pattern of StatusOr in the protobuf library. This will be used by ORTC factory methods; for instance, CreateRtpSender will either return an RtpSender or an error if the parameters are invalid or some other failure occurs. This CL also moves RTCError classes to a separate file, and adds tests that were missing before. BUG=webrtc:7013 ========== to ========== Adding RTCErrorOr class to be used by ORTC APIs. This utility class can be used to represent either an error or a successful return value. Follows the pattern of StatusOr in the protobuf library. This will be used by ORTC factory methods; for instance, CreateRtpSender will either return an RtpSender or an error if the parameters are invalid or some other failure occurs. This CL also moves RTCError classes to a separate file, and adds tests that were missing before. BUG=webrtc:7013 Review-Url: https://codereview.webrtc.org/2692723002 Cr-Commit-Position: refs/heads/master@{#16659} Committed: https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/6038e97e048bebf763f7149ce...
Message was sent while issue was closed.
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Message was sent while issue was closed.
https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc... webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") { Can you make this a rtc_source_set and add it as a dependency in rtc_unittests instead? https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a... I'd prefer not adding another test binary if possible. I don't see webrtc/api will have that many tests long-term.
Message was sent while issue was closed.
https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc... webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") { On 2017/02/21 10:48:03, kjellander_webrtc wrote: > Can you make this a rtc_source_set and add it as a dependency in rtc_unittests > instead? > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a... > > I'd prefer not adding another test binary if possible. I don't see webrtc/api > will have that many tests long-term. I'm neutral on the issue of having more test binaries or not, but webrtc/api/ will probably end up with a bunch of unit tests. Stuff like ArrayView, Optional, Buffer, checks.h, etc. are going to have to move to api/ sooner or later.
Message was sent while issue was closed.
https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc... webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") { On 2017/02/21 12:00:26, kwiberg-webrtc wrote: > On 2017/02/21 10:48:03, kjellander_webrtc wrote: > > Can you make this a rtc_source_set and add it as a dependency in rtc_unittests > > instead? > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a... > > > > I'd prefer not adding another test binary if possible. I don't see webrtc/api > > will have that many tests long-term. > > I'm neutral on the issue of having more test binaries or not, but webrtc/api/ > will probably end up with a bunch of unit tests. Stuff like ArrayView, Optional, > Buffer, checks.h, etc. are going to have to move to api/ sooner or later. Right, but at least I expect them all to be fast. Not stuff like ramping up bitrates and things like that. Please let's try to not introduce a new one here.
Message was sent while issue was closed.
https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn File webrtc/api/BUILD.gn (right): https://codereview.webrtc.org/2692723002/diff/160001/webrtc/api/BUILD.gn#newc... webrtc/api/BUILD.gn:206: rtc_test("rtc_api_unittests") { On 2017/02/21 12:01:44, kjellander_webrtc wrote: > On 2017/02/21 12:00:26, kwiberg-webrtc wrote: > > On 2017/02/21 10:48:03, kjellander_webrtc wrote: > > > Can you make this a rtc_source_set and add it as a dependency in > rtc_unittests > > > instead? > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?rcl=0bdacd8a... > > > > > > I'd prefer not adding another test binary if possible. I don't see > webrtc/api > > > will have that many tests long-term. > > > > I'm neutral on the issue of having more test binaries or not, but webrtc/api/ > > will probably end up with a bunch of unit tests. Stuff like ArrayView, > Optional, > > Buffer, checks.h, etc. are going to have to move to api/ sooner or later. > > Right, but at least I expect them all to be fast. Not stuff like ramping up > bitrates and things like that. > Please let's try to not introduce a new one here. Here: https://codereview.webrtc.org/2709573003 It was on my to-do list to do something about this anyway, since rtc_api_unittests isn't being run yet. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
