Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704483002:
Add a PrintTo function for rtc::Optional to aid with testing. (Closed)
DescriptionAdd a PrintTo function for rtc::Optional to aid with testing.
gtest can print objects if they have an operator<< or a PrintTo
function in the same namespace as the object's class. Since
std::optional does not seem to have an operator<<, it'd be preferable
not to rely on rtc::Optional being printable through operator<<.
Currently, gtest errors will just dump the raw bytes of
rtc::Optionals, which make them really annoying to work with in tests.
BUG=webrtc:7196
Review-Url: https://codereview.webrtc.org/2704483002
Cr-Commit-Position: refs/heads/master@{#16717}
Committed: https://chromium.googlesource.com/external/webrtc/+/e5c27a5db6dab92176f90c4a0cd5a77c6d166a58
Patch Set 1 #
Total comments: 16
Patch Set 2 : Clarified description, removed constructors, made PrintTo safer for sanitizers, removed static_assert #Patch Set 3 : Added a const variant of FunctionThatDoesNothing. #Patch Set 4 : Reworked PrintTo so that it's always implemented, or gtest will print out uninitialized bytes. #
Total comments: 7
Patch Set 5 : Added workaround for unused functions; removed attribute((unused)) #Patch Set 6 : Moved OptionalPrintObjectBytes into optional.h, added an #ifdef UNIT_TEST. #
Total comments: 8
Patch Set 7 : Removed unnecessary stuff. #Messages
Total messages: 56 (31 generated)
Description was changed from ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG= ========== to ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG= ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
More reviews! \o/ https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:305: -> decltype(static_cast<void>(*os << *opt)) { The SFINAE here is maybe not super-obvious if one's not worked with such constructs before. Should I explain it better? Essentially, the template will be skipped if *os << *opt isn't a valid expression, since the function will not have a valid return type. Regardless of the return type of that expression, I'm forcing the decltype() to be void, if the expression is at all valid, so the return type will be void. https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:767: static_assert(std::is_same<void, decltype(PrintTo(empty_printable, Not really the greatest place to do a static_assert of this, but doing it without having actual types in place is a mess. Also, it's probably unnecessary: the compiler complains about reaching the end of PrintTo without returning a value if its return type isn't void, so I might as well remove this. WDYT?
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/13563)
https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:305: -> decltype(static_cast<void>(*os << *opt)) { On 2017/02/16 13:20:05, ossu wrote: > The SFINAE here is maybe not super-obvious if one's not worked with such > constructs before. Should I explain it better? > Essentially, the template will be skipped if *os << *opt isn't a valid > expression, since the function will not have a valid return type. Regardless of > the return type of that expression, I'm forcing the decltype() to be void, if > the expression is at all valid, so the return type will be void. Yes, more explanation is probably called for. How about documenting what it is and how it behaves in one paragraph, and explaining the details on how it works in a second paragraph? (The explanation in your message here was good, I think.) https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:307: *os << *opt; This is test code, so maybe indicate that the value is contained in an optional? https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:28: int value; You can make these classes several lines shorter by hardcoding the numerical value... https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:767: static_assert(std::is_same<void, decltype(PrintTo(empty_printable, On 2017/02/16 13:20:05, ossu wrote: > Not really the greatest place to do a static_assert of this, but doing it > without having actual types in place is a mess. Also, it's probably unnecessary: > the compiler complains about reaching the end of PrintTo without returning a > value if its return type isn't void, so I might as well remove this. WDYT? Hmm. When would the return value not be void? https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:769: nullptr)))>::value != 0, std::declval<std::ostream*>() ?
https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:304: auto PrintTo(const Optional<T>& opt, std::ostream* os) Turns out the compiler does clever things that linux_memcheck doesn't like. I'll have to replicate what value_or() does to get it to stop complaining. https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:305: -> decltype(static_cast<void>(*os << *opt)) { On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > On 2017/02/16 13:20:05, ossu wrote: > > The SFINAE here is maybe not super-obvious if one's not worked with such > > constructs before. Should I explain it better? > > Essentially, the template will be skipped if *os << *opt isn't a valid > > expression, since the function will not have a valid return type. Regardless > of > > the return type of that expression, I'm forcing the decltype() to be void, if > > the expression is at all valid, so the return type will be void. > > Yes, more explanation is probably called for. How about documenting what it is > and how it behaves in one paragraph, and explaining the details on how it works > in a second paragraph? (The explanation in your message here was good, I think.) Will do! https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:307: *os << *opt; On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > This is test code, so maybe indicate that the value is contained in an optional? I considered that but was unsure if it would be more confusing than not. I think I'd rather not, if it's not very important. I'll think about it some more, though. https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:28: int value; On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > You can make these classes several lines shorter by hardcoding the numerical > value... That is true. I could also just use brace initialization down in the test. It's probably cleanest. https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:767: static_assert(std::is_same<void, decltype(PrintTo(empty_printable, On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > On 2017/02/16 13:20:05, ossu wrote: > > Not really the greatest place to do a static_assert of this, but doing it > > without having actual types in place is a mess. Also, it's probably > unnecessary: > > the compiler complains about reaching the end of PrintTo without returning a > > value if its return type isn't void, so I might as well remove this. WDYT? > > Hmm. When would the return value not be void? I put this here only to ensure that my cast in decltype() in optional.h actually worked. And it did! :) https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:769: nullptr)))>::value != 0, On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > std::declval<std::ostream*>() ? Ah, right! The last time I did this, I had a C++11 compiler but no C++11 standard library, so declval wasn't available. Will fix.
https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest.cc File webrtc/base/optional_unittest.cc (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional_unittest... webrtc/base/optional_unittest.cc:767: static_assert(std::is_same<void, decltype(PrintTo(empty_printable, On 2017/02/16 14:23:48, ossu wrote: > On 2017/02/16 14:07:37, kwiberg-webrtc wrote: > > On 2017/02/16 13:20:05, ossu wrote: > > > Not really the greatest place to do a static_assert of this, but doing it > > > without having actual types in place is a mess. Also, it's probably > > unnecessary: > > > the compiler complains about reaching the end of PrintTo without returning a > > > value if its return type isn't void, so I might as well remove this. WDYT? > > > > Hmm. When would the return value not be void? > > I put this here only to ensure that my cast in decltype() in optional.h actually > worked. And it did! :) Oh, OK, that explains why you were talking about warnings about reaching the end of a non-void function... So yeah, remove the static_assert then.
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/13569)
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/13574)
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/13626)
I've reworked PrintTo so that gtest shouldn't accidentally try to print uninitialized bytes. I was hoping to have it run through the bots first, before emailing, but they seem tired today.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:304: auto PrintTo(const Optional<T>& opt, std::ostream* os) Can we call this PrintToForTest? There's a risk that more and more ostream related things start to creep into the non-test code base and there is already a precedence for "ForTest" methods, including a presubmit check that prevents non-test code from calling those methods.
https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/1/webrtc/base/optional.h#newcod... webrtc/base/optional.h:304: auto PrintTo(const Optional<T>& opt, std::ostream* os) On 2017/02/17 13:46:23, tommi (webrtc) wrote: > Can we call this PrintToForTest? > > There's a risk that more and more ostream related things start to creep into the > non-test code base and there is already a precedence for "ForTest" methods, > including a presubmit check that prevents non-test code from calling those > methods. Alas, no, it needs to be called PrintTo for gtest to pick it up, c.f. https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
It would be nice if we could have these test-only things in a separate target, which we could then mark as test-only. But we have no good way to cause a compilation failure to let people know they need to include it, do we? https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h#ne... webrtc/base/optional.h:44: } Hmm. Wouldn't it be more general to just have one of these functions, template <typename T, typename std::enable_if<std::is_pointer<T>::value>::type* = nullptr> inline T FunctionThatDoesNothing(T x); defined only for types T that are pointers? Regardless, don't you need the const version in the other #if branch as well? https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h#ne... webrtc/base/optional.h:363: size_t size, ArrayView?
https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc File webrtc/base/optional.cc (right): https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc#n... webrtc/base/optional.cc:34: } This is just for testing, so you should probably inline it (making tests bigger, but production binaries smaller).
On 2017/02/17 13:50:17, kwiberg-webrtc wrote: > It would be nice if we could have these test-only things in a separate target, > which we could then mark as test-only. But we have no good way to cause a > compilation failure to let people know they need to include it, do we? Hmm. kjellander@, do we have a preprocessor constant that's set when compiling tests? gn allows us to mark targets as "testonly", so the information is there...
On 2017/02/17 13:50:17, kwiberg-webrtc wrote: > It would be nice if we could have these test-only things in a separate target, > which we could then mark as test-only. But we have no good way to cause a > compilation failure to let people know they need to include it, do we? This isn't _strictly_ a test only thing, though. It looks the way it does due to gtest, but I'd probably use it for regular debug printing of optionals, from time-to-time, when developing. Perhaps that would all be built in test mode as well? https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc File webrtc/base/optional.cc (right): https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc#n... webrtc/base/optional.cc:34: } On 2017/02/17 13:51:22, kwiberg-webrtc wrote: > This is just for testing, so you should probably inline it (making tests bigger, > but production binaries smaller). I'd rather keep this cruft as isolated as possible. It's only <iomanip> but it'd be nice to avoid including that in everything that uses Optional, if possible. I don't think the size increase is noticeable. https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h#ne... webrtc/base/optional.h:44: } On 2017/02/17 13:50:17, kwiberg-webrtc wrote: > Hmm. Wouldn't it be more general to just have one of these functions, > > template <typename T, > typename std::enable_if<std::is_pointer<T>::value>::type* = nullptr> > inline T FunctionThatDoesNothing(T x); > > defined only for types T that are pointers? > > Regardless, don't you need the const version in the other #if branch as well? Nope, the problem was that the compiler disallowed casting away const with reinterpret_cast. The other path doesn't do any casting, so it's fine regardless of type. However, I don't think I need it at all anymore, since I changed PrintTo to not use it. I'll remove it from here and put it back in if I follow up with a CL that ensures all references to value_ go through FunctionThatDoesNothing. https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.h#ne... webrtc/base/optional.h:363: size_t size, On 2017/02/17 13:50:17, kwiberg-webrtc wrote: > ArrayView? Not certain the extra dependency is worth it, honestly. It's just a helper for OptionalPrintToHelper, not a general interface. If it were, I would.
https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc File webrtc/base/optional.cc (right): https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc#n... webrtc/base/optional.cc:34: } On 2017/02/17 14:02:19, ossu wrote: > On 2017/02/17 13:51:22, kwiberg-webrtc wrote: > > This is just for testing, so you should probably inline it (making tests > bigger, > > but production binaries smaller). > > I'd rather keep this cruft as isolated as possible. It's only <iomanip> but it'd > be nice to avoid including that in everything that uses Optional, if possible. I > don't think the size increase is noticeable. My concern is that if we get to the point that we can do builds that are free of iostream, then just one place that pulls it in, will be enough to make a significant difference. If the argument is "it's just a drop in the ocean", then the counter argument is "death by a thousand cuts" :) Logging is very costly as it is, so we have our work cut out to address that and there are indeed situations where we've had to throw out all logging in order to be able to reach performance targets. If we add more things to battle, then we make the battle harder for ourselves in the end. So, whatever we can do to make utilities like these unavailable to production code, are appreciated. (if we could do what Karl suggests, make testing easier while making production code smaller, then that would be great)
On 2017/02/17 14:08:55, tommi (webrtc) wrote: > https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc > File webrtc/base/optional.cc (right): > > https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc#n... > webrtc/base/optional.cc:34: } > On 2017/02/17 14:02:19, ossu wrote: > > On 2017/02/17 13:51:22, kwiberg-webrtc wrote: > > > This is just for testing, so you should probably inline it (making tests > > bigger, > > > but production binaries smaller). > > > > I'd rather keep this cruft as isolated as possible. It's only <iomanip> but > it'd > > be nice to avoid including that in everything that uses Optional, if possible. > I > > don't think the size increase is noticeable. > > My concern is that if we get to the point that we can do builds that are free of > iostream, then just one place that pulls it in, will be enough to make a > significant difference. If the argument is "it's just a drop in the ocean", > then the counter argument is "death by a thousand cuts" :) > > Logging is very costly as it is, so we have our work cut out to address that and > there are indeed situations where we've had to throw out all logging in order to > be able to reach performance targets. > If we add more things to battle, then we make the battle harder for ourselves in > the end. > So, whatever we can do to make utilities like these unavailable to production > code, are appreciated. (if we could do what Karl suggests, make testing easier > while making production code smaller, then that would be great) Alright. I've moved the implementation back into the header file and put all of the PrintTo stuff within #ifdef UNIT_TEST blocks, including the <ostream> and <iomanip> includes. It only gets included when building a gtest target. I could move all the PrintTo stuff into its own header that gets conditionally included by optional.h depeding on if UNIT_TEST is defined. That could clean up optional.h a bit, but on the other hand make it easier to pull in PrintTo during development and then forget about it. WDYT? I imagine calling the file optional_for_testing.h or something.
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/13637)
On 2017/02/17 15:06:53, ossu wrote: > On 2017/02/17 14:08:55, tommi (webrtc) wrote: > > https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc > > File webrtc/base/optional.cc (right): > > > > > https://codereview.webrtc.org/2704483002/diff/60001/webrtc/base/optional.cc#n... > > webrtc/base/optional.cc:34: } > > On 2017/02/17 14:02:19, ossu wrote: > > > On 2017/02/17 13:51:22, kwiberg-webrtc wrote: > > > > This is just for testing, so you should probably inline it (making tests > > > bigger, > > > > but production binaries smaller). > > > > > > I'd rather keep this cruft as isolated as possible. It's only <iomanip> but > > it'd > > > be nice to avoid including that in everything that uses Optional, if > possible. > > I > > > don't think the size increase is noticeable. > > > > My concern is that if we get to the point that we can do builds that are free > of > > iostream, then just one place that pulls it in, will be enough to make a > > significant difference. If the argument is "it's just a drop in the ocean", > > then the counter argument is "death by a thousand cuts" :) > > > > Logging is very costly as it is, so we have our work cut out to address that > and > > there are indeed situations where we've had to throw out all logging in order > to > > be able to reach performance targets. > > If we add more things to battle, then we make the battle harder for ourselves > in > > the end. > > So, whatever we can do to make utilities like these unavailable to production > > code, are appreciated. (if we could do what Karl suggests, make testing easier > > while making production code smaller, then that would be great) > > Alright. I've moved the implementation back into the header file and put all of > the PrintTo stuff within #ifdef UNIT_TEST blocks, including the <ostream> and > <iomanip> includes. It only gets included when building a gtest target. > > I could move all the PrintTo stuff into its own header that gets conditionally > included by optional.h depeding on if UNIT_TEST is defined. That could clean up > optional.h a bit, but on the other hand make it easier to pull in PrintTo during > development and then forget about it. WDYT? I imagine calling the file > optional_for_testing.h or something. What you've done so far works for me. Appreciate going the extra mile on this btw.
On 2017/02/17 15:47:26, tommi (webrtc) wrote: > What you've done so far works for me. Appreciate going the extra mile on this > btw. Great, thanks! :)
https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:18: #ifdef UNIT_TEST Who defines this? I couldn't find it in our tree. https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:47: FunctionThatDoesNothingImpl(reinterpret_cast<void*>(const_cast<T*>(x))))); The outermost const_cast is unnecessary.
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:18: #ifdef UNIT_TEST On 2017/02/18 18:50:47, kwiberg-webrtc wrote: > Who defines this? I couldn't find it in our tree. It comes from gtest: https://cs.chromium.org/chromium/src/build/secondary/testing/gtest/BUILD.gn?r... so it should be defined for all that depend on //testing:gtest since it's a public config. I also noticed Chromium has this check: https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=22f522e10dd7a816ec45547... maybe we should add that too?
https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:18: #ifdef UNIT_TEST On 2017/02/19 09:29:19, kjellander_webrtc wrote: > On 2017/02/18 18:50:47, kwiberg-webrtc wrote: > > Who defines this? I couldn't find it in our tree. > > It comes from gtest: > https://cs.chromium.org/chromium/src/build/secondary/testing/gtest/BUILD.gn?r... > so it should be defined for all that depend on //testing:gtest since it's a > public config. Aha. Excellent! > I also noticed Chromium has this check: > https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=22f522e10dd7a816ec45547... > maybe we should add that too? Yes---I guess the problem is that .cc files are compiled with their own configs, which won't in general have UNIT_TEST defined? So ideally, this check should cover every preprocessor symbol set through a public_config.
https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:18: #ifdef UNIT_TEST On 2017/02/19 13:14:15, kwiberg-webrtc wrote: > On 2017/02/19 09:29:19, kjellander_webrtc wrote: > > On 2017/02/18 18:50:47, kwiberg-webrtc wrote: > > > Who defines this? I couldn't find it in our tree. > > > > It comes from gtest: > > > https://cs.chromium.org/chromium/src/build/secondary/testing/gtest/BUILD.gn?r... > > so it should be defined for all that depend on //testing:gtest since it's a > > public config. > > Aha. Excellent! > > > I also noticed Chromium has this check: > > > https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=22f522e10dd7a816ec45547... > > maybe we should add that too? > > Yes---I guess the problem is that .cc files are compiled with their own configs, > which won't in general have UNIT_TEST defined? So ideally, this check should > cover every preprocessor symbol set through a public_config. I don't have time to put in more work than this for now, so I'm just copying Chromium's check in https://codereview.webrtc.org/2703173002/
Alright. Anything else I've missed or are we fine with this? The two failing bots look like unrelated flakes. https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:47: FunctionThatDoesNothingImpl(reinterpret_cast<void*>(const_cast<T*>(x))))); On 2017/02/18 18:50:47, kwiberg-webrtc wrote: > The outermost const_cast is unnecessary. That is true. But also, I think the whole function might be. I'll remove it if it is.
Oh, right, I never stamped this. lgtm! https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.cc File webrtc/base/optional.cc (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.cc#n... webrtc/base/optional.cc:23: This extra newline is probably not necessary. Remove it if you feel like it... https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2704483002/diff/90001/webrtc/base/optional.h#ne... webrtc/base/optional.h:47: FunctionThatDoesNothingImpl(reinterpret_cast<void*>(const_cast<T*>(x))))); On 2017/02/20 11:02:24, ossu wrote: > On 2017/02/18 18:50:47, kwiberg-webrtc wrote: > > The outermost const_cast is unnecessary. > > That is true. But also, I think the whole function might be. I'll remove it if > it is. Acknowledged.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2704483002/#ps110001 (title: "Removed unnecessary stuff.")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13742)
Description was changed from ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG= ========== to ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG=webrtc:7196 ==========
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": 110001, "attempt_start_ts": 1487593697807620,
"parent_rev": "6bb8e0efd3187e11ac512d84d665b4cffacad43e", "commit_rev":
"e5c27a5db6dab92176f90c4a0cd5a77c6d166a58"}
Message was sent while issue was closed.
Description was changed from ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG=webrtc:7196 ========== to ========== Add a PrintTo function for rtc::Optional to aid with testing. gtest can print objects if they have an operator<< or a PrintTo function in the same namespace as the object's class. Since std::optional does not seem to have an operator<<, it'd be preferable not to rely on rtc::Optional being printable through operator<<. Currently, gtest errors will just dump the raw bytes of rtc::Optionals, which make them really annoying to work with in tests. BUG=webrtc:7196 Review-Url: https://codereview.webrtc.org/2704483002 Cr-Commit-Position: refs/heads/master@{#16717} Committed: https://chromium.googlesource.com/external/webrtc/+/e5c27a5db6dab92176f90c4a0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/external/webrtc/+/e5c27a5db6dab92176f90c4a0... |
