Make logging a scoped_ptr<T> more useful.

Make logging a scoped_ptr<T> more useful.

Given the code:
  scoped_ptr<std::string> x;
  LOG(ERROR) << x;
the logging output will always be 0 or 1. The reason is the
scoped_ptr<T> is getting turned into a bool before being logged. This
behavior doesn't seem very useful, so add a logging overload to make it
print out the pointer value.

BUG=none

Committed: https://crrev.com/12fcf61becc682d7d542532d7f50cf2406f25bdf
Cr-Commit-Position: refs/heads/master@{#316640}

[dcheng, 2015-01-07 01:22:55]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2015-01-07 01:22:55]

I added something similar for scoped_refptr<T> when removing the implicit
conversions. This seems strictly more useful than the current behavior.

[danakj, 2015-01-07 04:36:17]

On 2015/01/07 01:22:55, dcheng wrote:
> I added something similar for scoped_refptr<T> when removing the implicit
> conversions. This seems strictly more useful than the current behavior.

Shall we unit test it?

[dcheng, 2015-01-07 20:59:46]

Done.

[danakj, 2015-01-07 21:04:27]

Cool. One question, does this do anything to the build size?

[dcheng, 2015-01-08 00:36:25]

dcheng@chromium.org changed reviewers:
+ rdsmith@chromium.org

[dcheng, 2015-01-08 00:36:26]

This doesn't do anything to build size.

... however I discovered some interesting test code.

+rdsmith for the content/browser/download/ changes.

[danakj, 2015-01-08 00:52:50]

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

[danakj, 2015-01-08 00:52:50]

On 2015/01/08 00:36:26, dcheng wrote:
> This doesn't do anything to build size.

OK good, I wondered because jamesr went through some very large effort to avoid
adding <ostream> and these operators in the gfx:: geometry classes.

+jamesr to comment on this.

LGTM % jamesr

[dcheng, 2015-01-08 01:04:00]

My impression is that it was iostream that's problematic
(http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden is the
best reference I can find atm), but I could be wrong.

[jamesr, 2015-01-08 01:18:20]

It's not final binary size that's impacted so much as the input to the compiler
and intermediate translation unit size.  The linker should throw everything away
at the end, but it might take a lot longer or use a lot more memory (and we OOM
linkers fairly often).

For logging can't you out-of-line the implementation of this function and use
<iosfwd> in the header?

[jamesr, 2015-01-08 01:19:54]

you'd have to type-strip the T* somehow to void*, which might be tricky

[Randy Smith (Not in Mondays), 2015-01-08 16:39:35]

If the tests pass, content/browser/download/* LGTM--we don't use the mocking of
that method.

[danakj, 2015-01-08 19:12:13]

On 2015/01/08 01:19:54, jamesr wrote:
> you'd have to type-strip the T* somehow to void*, which might be tricky

This? Then you don't deref the ostream in the header?

// Member of the class.
std::ostream& scoped_ptr<T>::ToStream(std::ostream& out) {
  return out << get();
}

// Outside the class in the header.
template <typename T>
std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
  return p.ToStream(out);
}

[dcheng, 2015-01-08 22:33:17]

On 2015/01/08 at 19:12:13, danakj wrote:
> On 2015/01/08 01:19:54, jamesr wrote:
> > you'd have to type-strip the T* somehow to void*, which might be tricky
> 
> This? Then you don't deref the ostream in the header?
> 
> // Member of the class.
> std::ostream& scoped_ptr<T>::ToStream(std::ostream& out) {
>   return out << get();
> }
> 
> // Outside the class in the header.
> template <typename T>
> std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
>   return p.ToStream(out);
> }

I think this doesn't work, because in the .cc file, the compiler needs to know
what instantiations of scoped_ptr<T>::ToStream() to make.

I /think/ we could just static_cast it to void* (from
http://stackoverflow.com/questions/18929225/casting-class-pointer-to-void-poi...,
the relevant section of the standard indicates this is OK), and then pass it to
a helper to do the actual logging.

This ends up polluting base or base::subtle with a one-off logging helper that
should probably be reused by ref_counted.h and weak_ptr.h (if this is the route
we want to take).

[danakj, 2015-01-08 22:36:45]

On Thu, Jan 8, 2015 at 2:33 PM, <dcheng@chromium.org> wrote:

> On 2015/01/08 at 19:12:13, danakj wrote:
>
>> On 2015/01/08 01:19:54, jamesr wrote:
>> > you'd have to type-strip the T* somehow to void*, which might be tricky
>>
>
>  This? Then you don't deref the ostream in the header?
>>
>
>  // Member of the class.
>> std::ostream& scoped_ptr<T>::ToStream(std::ostream& out) {
>>    return out << get();
>> }
>>
>
>  // Outside the class in the header.
>> template <typename T>
>> std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
>>    return p.ToStream(out);
>> }
>>
>
> I think this doesn't work, because in the .cc file, the compiler needs to
> know
> what instantiations of scoped_ptr<T>::ToStream() to make.
>
> I /think/ we could just static_cast it to void* (from
> http://stackoverflow.com/questions/18929225/casting-
> class-pointer-to-void-pointer,
> the relevant section of the standard indicates this is OK), and then pass
> it to
> a helper to do the actual logging.
>
> This ends up polluting base or base::subtle with a one-off logging helper
> that
> should probably be reused by ref_counted.h and weak_ptr.h (if this is the
> route
> we want to take).
>

Sounds like it would be worth it to help us not kill linkers.
(base::internals maybe?)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2015-02-11 08:21:52]

Daniel, did this ever go anywhere?

For the linker issue, can we not rely on SFINAE and delayed instantiation to
still use iosfwd, but require the includer to include ostream (which they're
already bound to be doing by virtue of calling ToStream)

That is,

template <typename T>
std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
  return out << p.get();
}

Doesn't get instatiated in a CU until someone calls it, at which point it uses
the existing global naming scope. If the caller has included iostream, it should
"just work", right?

Meaning that you can still get away with things?

I can't recall the rules for instantiation to know if the p.ToStream(out) is
still valid, which is why I used .get() explicitly
(I would suspect not; when the compiler instantiates scoped_ptr<Concrete>,
doesn't it try to generate the code for scoped_ptr<Concrete>::ToStream() then,
and thus fail because iosfwd is forward declared and thus it's ambiguous whether
operator<< is defined. This would break any scoped_ptr<> users who don't have
ostream included, which would be bad)

[dcheng, 2015-02-16 19:49:41]

On 2015/02/11 at 08:21:52, rsleevi wrote:
> Daniel, did this ever go anywhere?
> 
> For the linker issue, can we not rely on SFINAE and delayed instantiation to
still use iosfwd, but require the includer to include ostream (which they're
already bound to be doing by virtue of calling ToStream)
> 
> That is,
> 
> template <typename T>
> std::ostream& operator<<(std::ostream& out, const scoped_ptr<T>& p) {
>   return out << p.get();
> }
> 
> Doesn't get instatiated in a CU until someone calls it, at which point it uses
the existing global naming scope. If the caller has included iostream, it should
"just work", right?
> 
> Meaning that you can still get away with things?
> 
> I can't recall the rules for instantiation to know if the p.ToStream(out) is
still valid, which is why I used .get() explicitly
> (I would suspect not; when the compiler instantiates scoped_ptr<Concrete>,
doesn't it try to generate the code for scoped_ptr<Concrete>::ToStream() then,
and thus fail because iosfwd is forward declared and thus it's ambiguous whether
operator<< is defined. This would break any scoped_ptr<> users who don't have
ostream included, which would be bad)

Ahhh. Good point.

I didn't realize this would work, because base/memory/ref_counted.h /appears/ to
pull in the ostream definition by #including base/logging.h. However, a careful
reader will realize that this is guarded by #ifndef NDEBUG, and the
std::ostream& operator<< overload for scoped_refptr has been checked in for
awhile (presumably without breaking the tree).

PTAL, base reviewers =)

[Nico, 2015-02-17 16:49:07]

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

[Nico, 2015-02-17 16:49:07]

lgtm :-|

[danakj, 2015-02-17 18:07:25]

LGTM

[dcheng, 2015-02-17 18:47:04]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-02-17 18:48:17]

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

[commit-bot: I haz the power, 2015-02-17 20:09:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-17 20:10:33]

Patchset 5 (id:??) landed as
https://crrev.com/12fcf61becc682d7d542532d7f50cf2406f25bdf
Cr-Commit-Position: refs/heads/master@{#316640}