Stop using some scoped_ptr features that unique_ptr doesn't have

webrtc/base/optional_unittest.cc

 /*
  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 47 matching lines @@
     Log2("operator==", a, b);
     return a.origin_ == b.origin_;
   }
   friend bool operator!=(const Logger& a, const Logger& b) {
     Log2("operator!=", a, b);
     return a.origin_ != b.origin_;
   }
   void Foo() { Log("Foo()"); }
   void Foo() const { Log("Foo() const"); }
   static rtc::scoped_ptr<std::vector<std::string>> Setup() {
-    auto s = rtc_make_scoped_ptr(new std::vector<std::string>);
+    auto s =
+        rtc::scoped_ptr<std::vector<std::string>>(new std::vector<std::string>);
     Logger::log_ = s.get();

[tommi, 2016/03/15 09:22:09]

Wow... Can this just return a vector instead? (move) 

[kwiberg-webrtc, 2016/03/15 09:59:41]

Yeah, I was about to send the author a sharply-worded note for not doing that in
the first place, but then I looked at the name of this file and realized who
must have written it...

Short version: It's done like this for a good reason. A pointer to the returned
vector is saved in a static member variable, where all Logger instances can get
at it (to write to the log). Each unit test begins by calling Setup, then
creates one or more Loggers that live and die, and at the end, the unit test
checks the log.

Of course, this ought to have been done with shared_ptr and weak_ptr rather than
manually making sure that the raw pointer to the vector isn't used after the
scoped_ptr goes out of scope, but the unit tests are small and simple.

[tommi (sloooow) - chröme, 2016/03/15 16:49:00]

OK.  Btw we shouldn't use auto when it actually makes things more verbose:
auto s = rtc::scoped_ptr<std::vector<std::string>>(new
std::vector<std::string>);
vs
rtc::scoped_ptr<std::vector<std::string>> s(new std::vector<std::string>);

It is very confusing that "Logger::log_" is a reference to a global variable.  I
didn't actually even notice the underscore at first.  To me it looked like a
variable declaration at first glance.  It should be just g_log (or s_log but I
think g_ is more commonly used) and since we're in the Logger class, we don't
have to further qualify the variable.
Same for next_id_.

Since this is static though and there's a global pointer kept that points to
something that's being returned via a scoped_ptr, I think that's not a good
design choice.  Feels like asking for dangling pointers to me.  Either the
ownership lies elsewhere entirely or it lies with this class entirely.  If here,
then the class could expose a TearDown method to free  the singleton.  Also, I
don't see a check for overwriting a possible previous value of log_.  Since the
tests rely on global data, we need to be careful to catch potentially bad state
left by previous runs (I'm assuming it's not possible to run these tests in
parallel as is).  If log_ is ever not null when we assign to it, we should
crash.

Anyway, getting back on track for this particular review, we can leave it as is
but feel free to remove the auto.

[kwiberg-webrtc, 2016/03/15 18:53:37]

Yeah, thanks for catching that. Fixed.
Sounds like all good ideas.
Yes. You mustn't forget to call Setup(). But without calling it you don't have a
log to verify, so it's not hard to remember.
But then we'd have to remember calling that instead.
True.
It's going to be non-null for all but the first test case to run in the same
process, isn't it? Overwriting is harmless as long as previous runs have
finished, since the static pointer isn't the owner.

Unfortunately we need the static pointer to the log, since we can't pass it as a
constructor argument. But I guess if we used a scoped_ptr with a custom deleter,
that deleter could check that the static pointer still pointed to the right
thing, then set it to null; Setup could then check that the static pointer
started out as null. That's the safest I can think of.

     Logger::next_id_ = 0;
     return s;
   }
 
  private:
   int id_;
   int origin_;
   static std::vector<std::string>* log_;
   static int next_id_;
   void Log(const char* msg) const {
@@ skipping 400 matching lines @@
               "4:17. copy constructor (from 0:17)", "5:5. default constructor",
               "6:6. default constructor", "7:7. default constructor", "---",
               "swap 2:42, 3:17", "swap 4:5, 5:17", "swap 6:7, 7:6", "---",
               "7:6. destructor", "6:7. destructor", "5:17. destructor",
               "4:5. destructor", "3:17. destructor", "2:42. destructor",
               "1:42. destructor", "0:17. destructor"),
             *log);
 }
 
 }  // namespace rtc