Move logging.h's definitions of operator<< into namespace std.

Move logging.h's definitions of operator<< into namespace std.

See the bug for a more detailed discussion of the problem when we don't make
this accessible through ADL.

BUG=391117
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281864

[Jeffrey Yasskin, 2014-07-02 23:58:56]

Happy July 4! PTAL when you get a chance, probably Monday or later.

[Nico, 2014-07-03 00:03:39]

I don't think undefined behavior is better than a few using statements. Pasting
the compiler error into google hopefully will find a previous thread about this.
(If not, paste the error you're seeing into a bug and reference that bug from a
CL that adds the using statement to the test.)

[awong, 2014-07-03 00:06:22]

Yeah...I kinda agree with thakis here. While this works, it doesn't seem to
be solving any particularly wide-spread problem.

Is there a reason these usings are distasteful enough that we'd consider
adding a symbol to namespace std?


On Wed, Jul 2, 2014 at 5:03 PM, <thakis@chromium.org> wrote:

> I don't think undefined behavior is better than a few using statements.
> Pasting
> the compiler error into google hopefully will find a previous thread about
> this.
> (If not, paste the error you're seeing into a bug and reference that bug
> from a
> CL that adds the using statement to the test.)
>
> https://codereview.chromium.org/367063006/
>

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

[Ryan Sleevi, 2014-07-03 00:31:16]

Yeah, the problem seems to come up maybe once a year (judging by echelog and
chromium-dev, where I've usually fielded the "You need using" question)

I'm also not a fan of slapping stuff into the std:: namespace.

[Jeffrey Yasskin, 2014-07-03 00:32:30]

The using's aren't really distasteful at all. I wrote the CL because
https://codereview.chromium.org/300573002/ got blocked for a month
because neither of us could figure out that the error message was
complaining about an ADL failure. Googling the Windows error would
have sped that up by finding rsleevi's thread, but we didn't think of
it. It's also not so bad to run into this when you're trying to
introduce a use of the badly-placed operator<<, but it's much worse
when you're trying to define your own operator<< and you're forced to
fix up a bunch of places that aren't even trying to use your new
operator.

The solution of placing the operator in namespace std is what google's
code picked, after talking to Matt Austern and other compiler experts
about what this undefined behavior was likely to mean:
https://goto.google.com/stl-logging-operator-in-std-namespace

Another solution would be to remove the operator<< entirely and make
callers cast to UTF-8 themselves. I'm not sure how many call sites
that'll be.

I do think we should remove places where people are going to trip over
ADL. It's just too obscure to make our contributors deal with.

On Wed, Jul 2, 2014 at 5:06 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org> wrote:
> Yeah...I kinda agree with thakis here. While this works, it doesn't seem to
> be solving any particularly wide-spread problem.
>
> Is there a reason these usings are distasteful enough that we'd consider
> adding a symbol to namespace std?
>
>
> On Wed, Jul 2, 2014 at 5:03 PM, <thakis@chromium.org> wrote:
>>
>> I don't think undefined behavior is better than a few using statements.
>> Pasting
>> the compiler error into google hopefully will find a previous thread about
>> this.
>> (If not, paste the error you're seeing into a bug and reference that bug
>> from a
>> CL that adds the using statement to the test.)
>>
>> https://codereview.chromium.org/367063006/
>
>

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

[awong, 2014-07-03 00:57:05]

How badly do you want to push for this?

I'm leaning towards not commit and putting this under the "won't fix: not
annoying enough" category.


On Wed, Jul 2, 2014 at 5:32 PM, Jeffrey Yasskin <jyasskin@chromium.org>
wrote:

> The using's aren't really distasteful at all. I wrote the CL because
> https://codereview.chromium.org/300573002/ got blocked for a month
> because neither of us could figure out that the error message was
> complaining about an ADL failure. Googling the Windows error would
> have sped that up by finding rsleevi's thread, but we didn't think of
> it. It's also not so bad to run into this when you're trying to
> introduce a use of the badly-placed operator<<, but it's much worse
> when you're trying to define your own operator<< and you're forced to
> fix up a bunch of places that aren't even trying to use your new
> operator.
>
> The solution of placing the operator in namespace std is what google's
> code picked, after talking to Matt Austern and other compiler experts
> about what this undefined behavior was likely to mean:
> https://goto.google.com/stl-logging-operator-in-std-namespace
>
> Another solution would be to remove the operator<< entirely and make
> callers cast to UTF-8 themselves. I'm not sure how many call sites
> that'll be.
>
> I do think we should remove places where people are going to trip over
> ADL. It's just too obscure to make our contributors deal with.
>
> On Wed, Jul 2, 2014 at 5:06 PM, Albert J. Wong (王重傑)
> <ajwong@chromium.org> wrote:
> > Yeah...I kinda agree with thakis here. While this works, it doesn't seem
> to
> > be solving any particularly wide-spread problem.
> >
> > Is there a reason these usings are distasteful enough that we'd consider
> > adding a symbol to namespace std?
> >
> >
> > On Wed, Jul 2, 2014 at 5:03 PM, <thakis@chromium.org> wrote:
> >>
> >> I don't think undefined behavior is better than a few using statements.
> >> Pasting
> >> the compiler error into google hopefully will find a previous thread
> about
> >> this.
> >> (If not, paste the error you're seeing into a bug and reference that bug
> >> from a
> >> CL that adds the using statement to the test.)
> >>
> >> https://codereview.chromium.org/367063006/
> >
> >
>

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

[Jeffrey Yasskin, 2014-07-03 01:02:10]

I'd be willing to try to delete the operators. I think this sort of
language corner case contributes to our bad scores on codebase
complexity, even if it doesn't produce confused threads that often,
and since it's avoidable, we should fix it.

On Wed, Jul 2, 2014 at 5:56 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org> wrote:
> How badly do you want to push for this?
>
> I'm leaning towards not commit and putting this under the "won't fix: not
> annoying enough" category.
>
>
> On Wed, Jul 2, 2014 at 5:32 PM, Jeffrey Yasskin <jyasskin@chromium.org>
> wrote:
>>
>> The using's aren't really distasteful at all. I wrote the CL because
>> https://codereview.chromium.org/300573002/ got blocked for a month
>> because neither of us could figure out that the error message was
>> complaining about an ADL failure. Googling the Windows error would
>> have sped that up by finding rsleevi's thread, but we didn't think of
>> it. It's also not so bad to run into this when you're trying to
>> introduce a use of the badly-placed operator<<, but it's much worse
>> when you're trying to define your own operator<< and you're forced to
>> fix up a bunch of places that aren't even trying to use your new
>> operator.
>>
>> The solution of placing the operator in namespace std is what google's
>> code picked, after talking to Matt Austern and other compiler experts
>> about what this undefined behavior was likely to mean:
>> https://goto.google.com/stl-logging-operator-in-std-namespace
>>
>> Another solution would be to remove the operator<< entirely and make
>> callers cast to UTF-8 themselves. I'm not sure how many call sites
>> that'll be.
>>
>> I do think we should remove places where people are going to trip over
>> ADL. It's just too obscure to make our contributors deal with.
>>
>> On Wed, Jul 2, 2014 at 5:06 PM, Albert J. Wong (王重傑)
>> <ajwong@chromium.org> wrote:
>> > Yeah...I kinda agree with thakis here. While this works, it doesn't seem
>> > to
>> > be solving any particularly wide-spread problem.
>> >
>> > Is there a reason these usings are distasteful enough that we'd consider
>> > adding a symbol to namespace std?
>> >
>> >
>> > On Wed, Jul 2, 2014 at 5:03 PM, <thakis@chromium.org> wrote:
>> >>
>> >> I don't think undefined behavior is better than a few using statements.
>> >> Pasting
>> >> the compiler error into google hopefully will find a previous thread
>> >> about
>> >> this.
>> >> (If not, paste the error you're seeing into a bug and reference that
>> >> bug
>> >> from a
>> >> CL that adds the using statement to the test.)
>> >>
>> >> https://codereview.chromium.org/367063006/
>> >
>> >
>
>

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

[awong, 2014-07-07 22:39:25]

I started a thread to poll the full set of base owners, but it turned into a
discussion so bringing it back here.

Arguments *against* modifying namespace std:
  - undefined behavior is undefined behavior
  - comparison to google3 flawed because our toolchain is more diverse and not
100% controlled by us
  - problem doesn't seem large enough to warrant this fix

Arguments *for* modifying namespace std:
  - pragmatically, main problem is possible ODR violation against runtime
library. Seems unlikely.
  - if it becomes a problem in the future, "fixing" things isn't hard (just add
bunches of usings).
  - It's part of the portable Google3 libraries which gives more exposure to
diverse toolchains.

What tilts me towards putting operator<< in namespace std is that, if a problem
arises, the fix is well scoped and not too hard. Combine that with the low
probability of unnoticed problems and I think we should allow this change.

-Albert



On 2014/07/03 01:02:10, Jeffrey Yasskin wrote:
> I'd be willing to try to delete the operators. I think this sort of
> language corner case contributes to our bad scores on codebase
> complexity, even if it doesn't produce confused threads that often,
> and since it's avoidable, we should fix it.
> 
> On Wed, Jul 2, 2014 at 5:56 PM, Albert J. Wong (王重傑)
> <mailto:ajwong@chromium.org> wrote:
> > How badly do you want to push for this?
> >
> > I'm leaning towards not commit and putting this under the "won't fix: not
> > annoying enough" category.
> >
> >
> > On Wed, Jul 2, 2014 at 5:32 PM, Jeffrey Yasskin
<mailto:jyasskin@chromium.org>
> > wrote:
> >>
> >> The using's aren't really distasteful at all. I wrote the CL because
> >> https://codereview.chromium.org/300573002/ got blocked for a month
> >> because neither of us could figure out that the error message was
> >> complaining about an ADL failure. Googling the Windows error would
> >> have sped that up by finding rsleevi's thread, but we didn't think of
> >> it. It's also not so bad to run into this when you're trying to
> >> introduce a use of the badly-placed operator<<, but it's much worse
> >> when you're trying to define your own operator<< and you're forced to
> >> fix up a bunch of places that aren't even trying to use your new
> >> operator.
> >>
> >> The solution of placing the operator in namespace std is what google's
> >> code picked, after talking to Matt Austern and other compiler experts
> >> about what this undefined behavior was likely to mean:
> >> https://goto.google.com/stl-logging-operator-in-std-namespace
> >>
> >> Another solution would be to remove the operator<< entirely and make
> >> callers cast to UTF-8 themselves. I'm not sure how many call sites
> >> that'll be.
> >>
> >> I do think we should remove places where people are going to trip over
> >> ADL. It's just too obscure to make our contributors deal with.
> >>
> >> On Wed, Jul 2, 2014 at 5:06 PM, Albert J. Wong (王重傑)
> >> <mailto:ajwong@chromium.org> wrote:
> >> > Yeah...I kinda agree with thakis here. While this works, it doesn't seem
> >> > to
> >> > be solving any particularly wide-spread problem.
> >> >
> >> > Is there a reason these usings are distasteful enough that we'd consider
> >> > adding a symbol to namespace std?
> >> >
> >> >
> >> > On Wed, Jul 2, 2014 at 5:03 PM, <mailto:thakis@chromium.org> wrote:
> >> >>
> >> >> I don't think undefined behavior is better than a few using statements.
> >> >> Pasting
> >> >> the compiler error into google hopefully will find a previous thread
> >> >> about
> >> >> this.
> >> >> (If not, paste the error you're seeing into a bug and reference that
> >> >> bug
> >> >> from a
> >> >> CL that adds the using statement to the test.)
> >> >>
> >> >> https://codereview.chromium.org/367063006/
> >> >
> >> >
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-07-08 00:03:19]

I'm on the "not worth it" side, but weakly. If awong or willchan want to approve
this, I can definitely live with that.

(Not approving it might provide incentive to fix the standard here, though,
given that the author is on the standards committee ;-) )

[Jeffrey Yasskin, 2014-07-08 00:31:40]

On Mon, Jul 7, 2014 at 5:03 PM,  <thakis@chromium.org> wrote:
> I'm on the "not worth it" side, but weakly. If awong or willchan want to
> approve
> this, I can definitely live with that.
>
> (Not approving it might provide incentive to fix the standard here, though,
> given that the author is on the standards committee ;-) )

Heh. I'm even on (in charge of) the right part of the committee to fix
this. The main problem is figuring out how to fix it. Do we want
mismatched ostream/string types to do a unicode conversion
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3336.html#Problem-2)?
Do we want the standard to define what happens for this sort of
operator definition? Something else? The auto-conversion has the issue
that some people still have non-utf8 system encodings, and there's no
existing way to distinguish them, so we get stuck in bikesheds
guessing what's going to surprise users least.

The timescale for defining ostream<<wstring is long enough, even if it
gets into C++17, that I'd want to fix this in the mean time. That
definition is actually what's most likely to break this CL in the
future. However, a definition of ostream<<wstring in the standard
would also make our current ::operator<< ambiguous, so we're not
losing anything there. When this does break us, we'll be able to use a
feature-testing macro
(http://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations)
to make the definitions conditional in the right cases.

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

[awong, 2014-07-08 00:33:42]

LGTM

Eng time discussing is starting to outweight eng time lost due to bugs.

https://codereview.chromium.org/367063006/diff/1/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/367063006/diff/1/base/logging.h#newcode849
base/logging.h:849: }
nit:

}  // namespace std

[willchan no longer on Chromium, 2014-07-08 00:35:48]

LGTM

Nico, if this comes back to bite us, I will award you 1 "I told you so!"
point. Redeemable for a free shot of whiskey.


On Mon, Jul 7, 2014 at 5:33 PM, <ajwong@chromium.org> wrote:

> LGTM
>
> Eng time discussing is starting to outweight eng time lost due to bugs.
>
>
> https://codereview.chromium.org/367063006/diff/1/base/logging.h
> File base/logging.h (right):
>
> https://codereview.chromium.org/367063006/diff/1/base/logging.h#newcode849
> base/logging.h:849: }
> nit:
>
> }  // namespace std
>
> https://codereview.chromium.org/367063006/
>

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

[willchan no longer on Chromium, 2014-07-08 00:36:14]

Haha, I always forget to use the web app to publish a LGTM.

[awong, 2014-07-08 00:38:34]

jyasskin: Just remember to commit manually, no redo of the try server, and
then go on vacation for 4 weeks.

https://www.youtube.com/watch?v=Sqz5dbs5zmo&feature=kp

(don't do that)


On Mon, Jul 7, 2014 at 5:36 PM, <willchan@chromium.org> wrote:

> Haha, I always forget to use the web app to publish a LGTM.
>
> https://codereview.chromium.org/367063006/
>

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

[Jeffrey Yasskin, 2014-07-08 00:42:39]

On Mon, Jul 7, 2014 at 5:38 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org> wrote:
> jyasskin: Just remember to commit manually, no redo of the try server, and
> then go on vacation for 4 weeks.

SG. I've dcommitted. I'll be on a tropical island with no laptop if you need
me.

> https://www.youtube.com/watch?v=Sqz5dbs5zmo&feature=kp
>
> (don't do that)
>
>
> On Mon, Jul 7, 2014 at 5:36 PM, <willchan@chromium.org> wrote:
>>
>> Haha, I always forget to use the web app to publish a LGTM.
>>
>> https://codereview.chromium.org/367063006/
>
>

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

[Ryan Sleevi, 2014-07-08 00:49:53]

Say hi to willchan while you're there, if you can find him.


On Mon, Jul 7, 2014 at 5:42 PM, Jeffrey Yasskin <jyasskin@chromium.org>
wrote:

> On Mon, Jul 7, 2014 at 5:38 PM, Albert J. Wong (王重傑)
> <ajwong@chromium.org> wrote:
> > jyasskin: Just remember to commit manually, no redo of the try server,
> and
> > then go on vacation for 4 weeks.
>
> SG. I've dcommitted. I'll be on a tropical island with no laptop if you
> need me.
>
> > https://www.youtube.com/watch?v=Sqz5dbs5zmo&feature=kp
> >
> > (don't do that)
> >
> >
> > On Mon, Jul 7, 2014 at 5:36 PM, <willchan@chromium.org> wrote:
> >>
> >> Haha, I always forget to use the web app to publish a LGTM.
> >>
> >> https://codereview.chromium.org/367063006/
> >
> >
>

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

[brettw, 2014-07-08 06:13:49]

When do we have wchar's floating around. If this was about char16/string16's it
would make more sense to me that somebody would want to do this (staying away
from whether it's a good idea or not). But we've been decreasing some wstring's,
I replaced a bunch a few months ago. I'm suspicious of any code that has this
problem. We should instead be changing such code to use the -16 variants.

[Jeffrey Yasskin, 2014-07-08 15:46:55]

On 2014/07/08 06:13:49, brettw wrote:
> When do we have wchar's floating around. If this was about char16/string16's
it
> would make more sense to me that somebody would want to do this (staying away
> from whether it's a good idea or not). But we've been decreasing some
wstring's,
> I replaced a bunch a few months ago. I'm suspicious of any code that has this
> problem. We should instead be changing such code to use the -16 variants.

Most of the uses of this operator that I found were to output FilePath::value().
We could change the PrintTo at
https://code.google.com/p/chromium/codesearch/#chromium/src/base/files/file_p...
into an operator<< and remove most of the need.

That said, you can't define an operator<< for string16 outside of the std
namespace either.

[Jeffrey Yasskin, 2014-07-08 17:32:23]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-07-08 17:33:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/367063006/20001

[commit-bot: I haz the power, 2014-07-08 23:03:10]

Change committed as 281864

[mitulblues, 2016-07-07 03:19:03]

On 2014/07/03 00:03:39, Nico wrote:
> I don't think undefined behavior is better than a few using statements.
Pasting
> the compiler error into google hopefully will find a previous thread about
this.
> (If not, paste the error you're seeing into a bug and reference that bug from
a
> CL that adds the using statement to the test.)