base: Add/Fix namespace closing comments.

base: Add/Fix namespace closing comments.

A namespace foo should end with a comment "// namespace foo", this
corrects errors in base/.

BUG=464816
Committed: https://crrev.com/c3762b9e20b93c8f94f44224b5023789173965e2
Cr-Commit-Position: refs/heads/master@{#319543}

[danakj, 2015-03-06 22:01:41]

Patchset #2 (id:20001) has been deleted

[danakj, 2015-03-06 22:01:56]

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

[danakj, 2015-03-06 22:02:06]

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

[Nico, 2015-03-06 22:11:17]

lgtm

This kind of thing, while strictly correct, seems inconsequential enough that
it's almost not worth fixing. I'm afraid that cpplint will make us focus on
things like this more than useful. (the guid->base change is good, i suppose)

(To be absolutely clear, I'm just sharing my thoughts on this. You're absolutely
free to think "well you're WRONG nico" and continue doing this, and I'll gladly
lgtm it all.)

https://codereview.chromium.org/981803003/diff/1/base/atomicops_internals_por...
File base/atomicops_internals_portable.h (left):

https://codereview.chromium.org/981803003/diff/1/base/atomicops_internals_por...
base/atomicops_internals_portable.h:225: }  // namespace base::subtle
totally valid c++17 :-P

[danakj, 2015-03-06 22:13:55]

On Fri, Mar 6, 2015 at 2:11 PM, <thakis@chromium.org> wrote:

> lgtm
>
> This kind of thing, while strictly correct, seems inconsequential enough
> that
> it's almost not worth fixing. I'm afraid that cpplint will make us focus on
> things like this more than useful. (the guid->base change is good, i
> suppose)
>
> (To be absolutely clear, I'm just sharing my thoughts on this. You're
> absolutely
> free to think "well you're WRONG nico" and continue doing this, and I'll
> gladly
> lgtm it all.)
>

I somewhat agree, it's a little bit trivial, most of the time. But I also
don't want to have to look at their closing braces and double check they
are correct when I review a patch. If we can make this all right, then we
can have tools point it out to patch writers when they get it wrong instead
of me having to do that.


>
>
> https://codereview.chromium.org/981803003/diff/1/base/
> atomicops_internals_portable.h
> File base/atomicops_internals_portable.h (left):
>
> https://codereview.chromium.org/981803003/diff/1/base/
> atomicops_internals_portable.h#oldcode225
> base/atomicops_internals_portable.h:225: }  // namespace base::subtle
> totally valid c++17 :-P
>
> https://codereview.chromium.org/981803003/
>

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

[Nico, 2015-03-06 22:20:41]

On Fri, Mar 6, 2015 at 2:13 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Fri, Mar 6, 2015 at 2:11 PM, <thakis@chromium.org> wrote:
>
>> lgtm
>>
>> This kind of thing, while strictly correct, seems inconsequential enough
>> that
>> it's almost not worth fixing. I'm afraid that cpplint will make us focus
>> on
>> things like this more than useful. (the guid->base change is good, i
>> suppose)
>>
>> (To be absolutely clear, I'm just sharing my thoughts on this. You're
>> absolutely
>> free to think "well you're WRONG nico" and continue doing this, and I'll
>> gladly
>> lgtm it all.)
>>
>
> I somewhat agree, it's a little bit trivial, most of the time. But I also
> don't want to have to look at their closing braces and double check they
> are correct when I review a patch. If we can make this all right, then we
> can have tools point it out to patch writers when they get it wrong instead
> of me having to do that.
>

I guess my point is if this really isn't so important, maybe it doesn't
have to be checked by a tool either. * shrug *


>
>
>>
>>
>> https://codereview.chromium.org/981803003/diff/1/base/
>> atomicops_internals_portable.h
>> File base/atomicops_internals_portable.h (left):
>>
>> https://codereview.chromium.org/981803003/diff/1/base/
>> atomicops_internals_portable.h#oldcode225
>> base/atomicops_internals_portable.h:225: }  // namespace base::subtle
>> totally valid c++17 :-P
>>
>> https://codereview.chromium.org/981803003/
>>
>
>

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

[danakj, 2015-03-06 22:25:23]

On Fri, Mar 6, 2015 at 2:20 PM, Nico Weber <thakis@chromium.org> wrote:

> On Fri, Mar 6, 2015 at 2:13 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> On Fri, Mar 6, 2015 at 2:11 PM, <thakis@chromium.org> wrote:
>>
>>> lgtm
>>>
>>> This kind of thing, while strictly correct, seems inconsequential enough
>>> that
>>> it's almost not worth fixing. I'm afraid that cpplint will make us focus
>>> on
>>> things like this more than useful. (the guid->base change is good, i
>>> suppose)
>>>
>>> (To be absolutely clear, I'm just sharing my thoughts on this. You're
>>> absolutely
>>> free to think "well you're WRONG nico" and continue doing this, and I'll
>>> gladly
>>> lgtm it all.)
>>>
>>
>> I somewhat agree, it's a little bit trivial, most of the time. But I also
>> don't want to have to look at their closing braces and double check they
>> are correct when I review a patch. If we can make this all right, then we
>> can have tools point it out to patch writers when they get it wrong instead
>> of me having to do that.
>>
>
> I guess my point is if this really isn't so important, maybe it doesn't
> have to be checked by a tool either. * shrug *
>

Yah, kinda, but then why have it in the style guide at all? I think there
is value to doing this stuff, it makes reading the code nicer. So it should
be right. I think :)


>
>
>>
>>
>>>
>>>
>>> https://codereview.chromium.org/981803003/diff/1/base/
>>> atomicops_internals_portable.h
>>> File base/atomicops_internals_portable.h (left):
>>>
>>> https://codereview.chromium.org/981803003/diff/1/base/
>>> atomicops_internals_portable.h#oldcode225
>>> base/atomicops_internals_portable.h:225: }  // namespace base::subtle
>>> totally valid c++17 :-P
>>>
>>> https://codereview.chromium.org/981803003/
>>>
>>
>>
>

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

[danakj, 2015-03-06 22:28:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-03-06 22:30:05]

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

[rvargas (doing something else), 2015-03-06 22:53:43]

lgtm

https://codereview.chromium.org/981803003/diff/1/base/threading/thread_restri...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/981803003/diff/1/base/threading/thread_restri...
base/threading/thread_restrictions.h:46: }  // namespace content
I'm fine either way, but this is an interesting example for the conversation:

The style guide would require all namespaces to be terminated with a comment
(although even the style guide have at least one example that don't follow that
rule), but the tool clearly considers some things in this file simple enough to
not require the comment...

and given that better readability is the whole point, I'd argue that a few empty
lines in this code will do more for readability than the comment closing the
namespace. Specifically this line reads better without the comment, or with the
comment but an empty line after it, than with comment and no separation from the
next line.

[danakj, 2015-03-06 22:59:18]

https://codereview.chromium.org/981803003/diff/1/base/threading/thread_restri...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/981803003/diff/1/base/threading/thread_restri...
base/threading/thread_restrictions.h:46: }  // namespace content
On 2015/03/06 22:53:42, rvargas wrote:
> I'm fine either way, but this is an interesting example for the conversation:
> 
> The style guide would require all namespaces to be terminated with a comment
> (although even the style guide have at least one example that don't follow
that
> rule), but the tool clearly considers some things in this file simple enough
to
> not require the comment...
> 
> and given that better readability is the whole point, I'd argue that a few
empty
> lines in this code will do more for readability than the comment closing the
> namespace. Specifically this line reads better without the comment, or with
the
> comment but an empty line after it, than with comment and no separation from
the
> next line.

Ya this one is weird, I agree. It went over the "obvious where the start of the
namespace" threshold. Personally I am happy to accept something like this to
avoid having to think about it when writing/reviewing CLs though.

Ideally the tool could say "everything inside this namespace is forward
declarations" and ignore it though, IMHO.

[commit-bot: I haz the power, 2015-03-07 00:03:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:03:13]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[danakj, 2015-03-07 00:05:13]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:06:29]

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

[commit-bot: I haz the power, 2015-03-07 01:52:13]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-07 01:52:56]

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