Fix missing .Pass() on returning scoped_ptr<SdchManager::DictionarySet>

Fix missing .Pass() on returning scoped_ptr<SdchManager::DictionarySet>

Warns on VS2015. I'm not sure why this is new, I guess something
become move-able (or not-move-able).

d:\src\cr3\src\net\base\sdch_manager.cc(459): error C2248: 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr': cannot access private
member declared in class 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>'
        with
        [
            T=net::SdchManager::DictionarySet
        ]
d:\src\cr3\src\base\memory\scoped_ptr.h(312): note: see declaration of 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr'
        with
        [
            T=net::SdchManager::DictionarySet
        ]
d:\src\cr3\src\net\base\sdch_manager.cc(463): error C2248: 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr': cannot access private
member declared in class 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>'
        with
        [
            T=net::SdchManager::DictionarySet
        ]
d:\src\cr3\src\base\memory\scoped_ptr.h(312): note: see declaration of 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr'
        with
        [
            T=net::SdchManager::DictionarySet
        ]
d:\src\cr3\src\net\base\sdch_manager.cc(467): error C2248: 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr': cannot access private
member declared in class 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>'
        with
        [
            T=net::SdchManager::DictionarySet
        ]
d:\src\cr3\src\base\memory\scoped_ptr.h(312): note: see declaration of 'scoped_ptr<net::SdchManager::DictionarySet,base::DefaultDeleter<T>>::scoped_ptr'
        with
        [
            T=net::SdchManager::DictionarySet
        ]

R=rsleevi@chromium.org
BUG=440500
Committed: https://crrev.com/d9afd5483d99d09ff383d77428195a00141a9d55
Cr-Commit-Position: refs/heads/master@{#313399}

[scottmg, 2015-01-27 22:37:32]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-01-27 22:37:35]

The CQ bit was unchecked by scottmg@chromium.org

[Ilya Sherman, 2015-01-27 22:45:51]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ryan Sleevi, 2015-01-27 23:05:25]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-01-27 23:05:26]

lgtm

[commit-bot: I haz the power, 2015-01-27 23:06:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883753004/1

[commit-bot: I haz the power, 2015-01-27 23:36:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-01-27 23:37:30]

Patchset 1 (id:??) landed as
https://crrev.com/d9afd5483d99d09ff383d77428195a00141a9d55
Cr-Commit-Position: refs/heads/master@{#313399}

[jamesr, 2015-01-27 23:40:13]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2015-01-27 23:40:13]

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:459: return result.Pass();
hmm, this should be an xvalue and thus not require a .Pass().  Have you filed an
msdn bug?

[scottmg, 2015-01-27 23:42:11]

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:459: return result.Pass();
On 2015/01/27 23:40:13, jamesr wrote:
> hmm, this should be an xvalue and thus not require a .Pass().  Have you filed
an
> msdn bug?

Sorry, I don't fully understand those words as well as I ought to. This
http://www.chromium.org/developers/smart-pointer-guidelines is incorrect now
then for C++11?

[jamesr, 2015-01-27 23:45:38]

Those guidelines are fine, but this construct should compile even without the
.Pass() (just like saying .Pass().Pass().Pass() here would compile but isn't
necessary).  If MSVS is buggy here then this'l make life hard, but maybe my
understanding is wrong.

Nico, Jeffrey - should this return-of-local be considered an xvalue?  If so why
isn't it choosing the rvalue ref constructor?

[Nico, 2015-01-27 23:51:32]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-01-27 23:51:32]

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:459: return result.Pass();
On 2015/01/27 23:42:10, scottmg wrote:
> On 2015/01/27 23:40:13, jamesr wrote:
> > hmm, this should be an xvalue and thus not require a .Pass().  Have you
filed
> an
> > msdn bug?
> 
> Sorry, I don't fully understand those words as well as I ought to. This
> http://www.chromium.org/developers/smart-pointer-guidelines is incorrect now
> then for C++11?

jamesr: I think this is correct. This doesn't build either:

> type foo.cc
#include <memory>

std::unique_ptr<int> GetInt(bool b) {
  std::unique_ptr<int> result;
  if (b)
    return result;
  return result;
}

> cl /c foo.cc

Mith msvs2015, does that error too? It probably does. One has to std::move() the
returned unique_ptr. So it makes sense to me that one has to .Pass() a
scoped_ptr. (written in 1 minute without much research tho)

[scottmg, 2015-01-27 23:53:42]

On 2015/01/27 23:51:32, Nico wrote:
> https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
> net/base/sdch_manager.cc:459: return result.Pass();
> On 2015/01/27 23:42:10, scottmg wrote:
> > On 2015/01/27 23:40:13, jamesr wrote:
> > > hmm, this should be an xvalue and thus not require a .Pass().  Have you
> filed
> > an
> > > msdn bug?
> > 
> > Sorry, I don't fully understand those words as well as I ought to. This
> > http://www.chromium.org/developers/smart-pointer-guidelines is incorrect now
> > then for C++11?
> 
> jamesr: I think this is correct. This doesn't build either:
> 
> > type foo.cc
> #include <memory>
> 
> std::unique_ptr<int> GetInt(bool b) {
>   std::unique_ptr<int> result;
>   if (b)
>     return result;
>   return result;
> }
> 
> > cl /c foo.cc
> 
> Mith msvs2015, does that error too? It probably does. One has to std::move()
the
> returned unique_ptr. So it makes sense to me that one has to .Pass() a
> scoped_ptr. (written in 1 minute without much research tho)

Hm, no, that doesn't error on 2015.

[scottmg, 2015-01-27 23:55:02]

On 2015/01/27 23:53:42, scottmg wrote:
> On 2015/01/27 23:51:32, Nico wrote:
> > https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
> > File net/base/sdch_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
> > net/base/sdch_manager.cc:459: return result.Pass();
> > On 2015/01/27 23:42:10, scottmg wrote:
> > > On 2015/01/27 23:40:13, jamesr wrote:
> > > > hmm, this should be an xvalue and thus not require a .Pass().  Have you
> > filed
> > > an
> > > > msdn bug?
> > > 
> > > Sorry, I don't fully understand those words as well as I ought to. This
> > > http://www.chromium.org/developers/smart-pointer-guidelines is incorrect
now
> > > then for C++11?
> > 
> > jamesr: I think this is correct. This doesn't build either:
> > 
> > > type foo.cc
> > #include <memory>
> > 
> > std::unique_ptr<int> GetInt(bool b) {
> >   std::unique_ptr<int> result;
> >   if (b)
> >     return result;
> >   return result;
> > }
> > 
> > > cl /c foo.cc
> > 
> > Mith msvs2015, does that error too? It probably does. One has to std::move()
> the
> > returned unique_ptr. So it makes sense to me that one has to .Pass() a
> > scoped_ptr. (written in 1 minute without much research tho)
> 
> Hm, no, that doesn't error on 2015.

(or 2013 for that matter.)

So I guess there's something about our scoped_ptr that it doesn't like?

[jamesr, 2015-01-27 23:55:08]

On 2015/01/27 23:51:32, Nico wrote:
> https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
> net/base/sdch_manager.cc:459: return result.Pass();
> On 2015/01/27 23:42:10, scottmg wrote:
> > On 2015/01/27 23:40:13, jamesr wrote:
> > > hmm, this should be an xvalue and thus not require a .Pass().  Have you
> filed
> > an
> > > msdn bug?
> > 
> > Sorry, I don't fully understand those words as well as I ought to. This
> > http://www.chromium.org/developers/smart-pointer-guidelines is incorrect now
> > then for C++11?
> 
> jamesr: I think this is correct. This doesn't build either:
> 
> > type foo.cc
> #include <memory>
> 
> std::unique_ptr<int> GetInt(bool b) {
>   std::unique_ptr<int> result;
>   if (b)
>     return result;
>   return result;
> }
> 
> > cl /c foo.cc
> 

Right, I'm saying I think that is a bug.  clang is happy with both of these:


std::unique_ptr<int> GetInt(bool b) {
  std::unique_ptr<int> result;
  if (b)
    return result;
  return result;
}

std::unique_ptr<int> GetInt2() {
  std::unique_ptr<int> result;
  return result;
}

GetInt2 should be pretty obviously correct - that's the canonical example of an
xvalue which binds to rvalue ref c'tors.  Does that error out too with cl.exe ?


> Mith msvs2015, does that error too? It probably does. One has to std::move()
the
> returned unique_ptr. So it makes sense to me that one has to .Pass() a
> scoped_ptr. (written in 1 minute without much research tho)

[jamesr, 2015-01-27 23:57:24]

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
net/base/sdch_manager.cc:459: return result.Pass();
come to think of it, why aren't these just "return nullptr" ?  maybe NRVO is
messing this up.

what do msvs2013/2015 do with this:

std::unique_ptr<int> GetInt(bool b) {
  std::unique_ptr<int> one;
  std::unique_ptr<int> two;
  if (b)
    return one;
  return two;
}

?

[scottmg, 2015-01-27 23:59:57]

On 2015/01/27 23:57:24, jamesr wrote:
> https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
> File net/base/sdch_manager.cc (right):
> 
>
https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
> net/base/sdch_manager.cc:459: return result.Pass();
> come to think of it, why aren't these just "return nullptr" ?  maybe NRVO is
> messing this up.
> 
> what do msvs2013/2015 do with this:
> 
> std::unique_ptr<int> GetInt(bool b) {
>   std::unique_ptr<int> one;
>   std::unique_ptr<int> two;
>   if (b)
>     return one;
>   return two;
> }
> 
> ?

2013 and 2015 both accept that.

[scottmg, 2015-01-28 00:54:32]

On 2015/01/27 23:59:57, scottmg wrote:
> On 2015/01/27 23:57:24, jamesr wrote:
> > https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc
> > File net/base/sdch_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/883753004/diff/1/net/base/sdch_manager.cc#new...
> > net/base/sdch_manager.cc:459: return result.Pass();
> > come to think of it, why aren't these just "return nullptr" ?  maybe NRVO is
> > messing this up.
> > 
> > what do msvs2013/2015 do with this:
> > 
> > std::unique_ptr<int> GetInt(bool b) {
> >   std::unique_ptr<int> one;
> >   std::unique_ptr<int> two;
> >   if (b)
> >     return one;
> >   return two;
> > }
> > 
> > ?
> 
> 2013 and 2015 both accept that.

Filed https://connect.microsoft.com/VisualStudio/feedback/details/1105046. I'll
revert this pending the resolution of that bug.