[C++11 Allowed Features] Declares a type-safe null pointer converting from NULL to nullptr [part-4]

[C++11 Allowed Features] Declares a type-safe null pointer converting from NULL to  nullptr [part-4]

BUG =

[ARUNKK, 2014-10-09 09:19:32]

kulkarni.a@samsung.com changed reviewers:
+ danakj@chromium.org

[ARUNKK, 2014-10-09 09:19:33]

PTAL.

[danakj, 2014-10-09 16:01:14]

please fix comments and run git cl format

[ARUNKK, 2014-10-10 06:07:07]

On 2014/10/09 16:01:14, danakj wrote:
> please fix comments and run git cl format

Thanks for review.Not found any presubmit  violations in the patch.In other CQ
i'm rectifying the same.

[ARUNKK, 2014-10-10 12:51:19]

kulkarni.a@samsung.com changed reviewers:
+ reveman@chromium.cor

[ARUNKK, 2014-10-10 12:51:20]

PTAL.

[ARUNKK, 2014-10-10 12:52:05]

kulkarni.a@samsung.com changed reviewers:
+ reveman@chromium.org
- reveman@chromium.cor

[ARUNKK, 2014-10-10 12:52:06]

PTAL.

[Avi (use Gerrit), 2014-10-10 16:55:26]

Your change description makes no sense. "[C++11 Allowed Features] Declares a
type-safe null pointer converting from NULL to  nullptr". This change does not
declare any pointers. This description seems to mash together two sentences:
"C++11 declares a type safe null pointer" and "This change converts from NULL to
nullptr". Please rewrite it to be grammatical English.

Also, if we’re doing a bulk conversion from NULL to nullptr, we should have a
master BUG tying this all together.

[danakj, 2014-10-10 16:57:07]

On 2014/10/10 16:55:26, Avi wrote:
> Your change description makes no sense. "[C++11 Allowed Features] Declares a
> type-safe null pointer converting from NULL to  nullptr". This change does not
> declare any pointers. This description seems to mash together two sentences:
> "C++11 declares a type safe null pointer" and "This change converts from NULL
to
> nullptr". Please rewrite it to be grammatical English.
> 
> Also, if we’re doing a bulk conversion from NULL to nullptr, we should have a
> master BUG tying this all together.

+1

Also please follow these directions when making a description:
http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html

LGTM once these are resolved.

[viettrungluu, 2014-10-10 17:16:20]

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

[viettrungluu, 2014-10-10 17:16:20]

https://codereview.chromium.org/634243003/diff/30001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/634243003/diff/30001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.cc:134: dict->SetString("observer", "nullptr");
WAT

https://codereview.chromium.org/634243003/diff/30001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.cc:458: dict->SetString("active_source",
"nullptr");
"

[danakj, 2014-10-10 17:18:53]

https://codereview.chromium.org/634243003/diff/30001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/634243003/diff/30001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_source.cc:134: dict->SetString("observer", "nullptr");
On 2014/10/10 17:16:20, viettrungluu wrote:
> WAT

This will change the output of tracing to show nullptr instead of NULL. Idk that
either is better than the other in this case.

You're right that nullptr in a comment is usually better written as null though,
I need to go back and look for that.

[danakj, 2014-10-10 17:27:35]

https://codereview.chromium.org/634243003/diff/30001/cc/test/test_web_graphic...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/634243003/diff/30001/cc/test/test_web_graphic...
cc/test/test_web_graphics_context_3d.h:290: // When set, mapImageCHROMIUM and
mapBufferCHROMIUM will return nullptr after
just use null in comments

[viettrungluu, 2014-10-10 17:32:06]

On Fri, Oct 10, 2014 at 10:18 AM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/634243003/diff/30001/cc/
> scheduler/begin_frame_source.cc
> File cc/scheduler/begin_frame_source.cc (right):
>
> https://codereview.chromium.org/634243003/diff/30001/cc/
> scheduler/begin_frame_source.cc#newcode134
> cc/scheduler/begin_frame_source.cc:134: dict->SetString("observer",
> "nullptr");
> On 2014/10/10 17:16:20, viettrungluu wrote:
>
>> WAT
>>
>
> This will change the output of tracing to show nullptr instead of NULL.
> Idk that either is better than the other in this case.
>

Fair enough, but such changes require individual consideration and probably
don't belong in a generic NULL -> nullptr change.


>
> You're right that nullptr in a comment is usually better written as null
> though, I need to go back and look for that.
>
> https://codereview.chromium.org/634243003/
>

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

[danakj, 2014-10-10 17:50:30]

On Fri, Oct 10, 2014 at 1:32 PM, Viet-Trung Luu <viettrungluu@chromium.org>
wrote:

> On Fri, Oct 10, 2014 at 10:18 AM, <danakj@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/634243003/diff/30001/cc/
>> scheduler/begin_frame_source.cc
>> File cc/scheduler/begin_frame_source.cc (right):
>>
>> https://codereview.chromium.org/634243003/diff/30001/cc/
>> scheduler/begin_frame_source.cc#newcode134
>> cc/scheduler/begin_frame_source.cc:134: dict->SetString("observer",
>> "nullptr");
>> On 2014/10/10 17:16:20, viettrungluu wrote:
>>
>>> WAT
>>>
>>
>> This will change the output of tracing to show nullptr instead of NULL.
>> Idk that either is better than the other in this case.
>>
>
> Fair enough, but such changes require individual consideration and
> probably don't belong in a generic NULL -> nullptr change.
>

Ya, perhaps. We keep a pretty low bar on providing a stable output to trace
viewer over time. I feel pretty ambivalent about it, but you're probably
right, we should leave it as "NULL" for now.

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

[Avi (use Gerrit), 2014-10-10 21:17:13]

Peter wrote up a guideline as to doing these kinds of changes:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/As26W6SbxQU

[Avi (use Gerrit), 2014-10-24 21:09:16]

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

[Avi (use Gerrit), 2014-10-24 21:10:26]

On 2014/10/10 21:17:13, Avi wrote:
> Peter wrote up a guideline as to doing these kinds of changes:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/As26W6SbxQU

Things seem to have stalled out here. I want to explicitly state not lgtm to
cancel out danakj@'s lg, to prevent accidental submission. When you're ready to
update, ping me and I'll take a look at any new patchsets.