Re: [chromium-dev] Re: C++11 Feature Proposal: Allowing/Encouraging use of nullptr

[Mounir Lamouri]

+blin...@chromium.org

I assume that the decision would apply to Chromium and Blink, right?

On Wed, 24 Sep 2014, at 10:21, Ryan Sleevi wrote:
> On Tue, Sep 23, 2014 at 5:18 PM, James Robinson <jam...@chromium.org>
> wrote:
>
> > On Tue, Sep 23, 2014 at 5:14 PM, Ryan Sleevi <rsl...@chromium.org> wrote:
> >
> >>
> >>
> >> On Tue, Sep 23, 2014 at 5:09 PM, Antoine Labour <pi...@google.com> wrote:
> >>
> >>>
> >>> Can it make sense to have a typedef decltype(nullptr) nullptr_t; in the
> >>> (say) base namespace, until we can use std::nullptr_t?
> >>>
> >>
> > We could, but I'm not sure how often it would be used and having a
> > base::nullptr_t typedef could be even more confusing. I think we need to
> > look at the set of use cases for this to decide if a base:: typedef or just
> > saying decltype(nullptr) is cleaner. I'm having trouble imagining a
> > tremendously large number of uses of base::nullptr_t. In blink,
> > std::nullptr_t is only used in 14 files, including the ones that define
> > it. If we need it lots of times, we can add a base:: version. If we only
> > need it a few times in some base types - say just in logging.h, scoped_ptr
> > and (maybe) weak_ptr - it's probably not worth the overhead of another type.
> >
> >
> >>
> >>> Antoine
> >>>
> >>
> >> The only reason we should need the nullptr_t type would be for template
> >> disambiguation.
> >>
> >> This assumes that decltype(nullptr) is legal (although not necessarily
> >> any decltype), in part, because it's what we've been doing in Blink for
> >> some time (which decltype's into... std::nullptr_t :'( )
> >>
> >
> > decltype(nullptr) is legal syntax. Blink's emulation is conditional so it
> > only compiles the std::nullptr_t in for toolchains that do not have library
> > support for it. It's technically undefined behavior to declare a type in
> > std in this way, although it's safe so far, but the fact that blink
> > declares the typedef in std means that we can't do the same outside of
> > Blink without generating ODR violations.
> >
> > - James
> >
> >
> Sorry, I meant "legal" in the sense of "An allowed exception to the
> decltype caveats earlier in this thread".
>
> Put differently, even if we ban decltype() in general (because there
> might
> be toolchain issues, which was what was alluded to), decltype(nullptr)
> would be a limited exception. I also anticipate it being extremely
> limited,
> and something that, if it absolutely needed to be a type, would probably
> be
> something in base::internal rather than base::
>
> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-dev...@chromium.org.

[James Robinson]

Blink is already using nullptr extensively via emulation. This would have relatively small impact on most of the Blink codebase except for in places where the WTF emulation can't be used, such as the Blink public c++ api, where the impact will probably be fairly small.

- James

[Nico Weber]

Since folks from all time zones had a chance to chime in, I'll attempt a summary:

* Nobody's opposed to using nullptr. It has a few advantages over NULL (works in gtest macros, works better with varargs). We should encourage using nullptr instead of NULL for new code.

* We may or may not add a base::nullptr_t, it probably won't be needed often.

* We won't mass-replace NULL with nullptr (only one person asked for this, it makes blame output harder to parse, and lines containing NULL are often tricky and interesting in blame output).

rsleevi, do you want to send a CL to change src/styleguide/c++/c++11.html to move nullptr to the explicitly allowed bucket?

[Nico Weber]

This is now live: http://chromium-cpp.appspot.com/

[Peter Kasting]

On Wed, Sep 24, 2014 at 12:04 PM, Nico Weber <tha...@chromium.org> wrote:

* We won't mass-replace NULL with nullptr (only one person asked for this, it makes blame output harder to parse, and lines containing NULL are often tricky and interesting in blame output).

Actually I'd like this too.  There are going to be other cases (e.g. the OVERRIDE transformation) where we're going to make mass changes to the codebase, and I think we should always optimize for consistency and clarity and never for blame -- it's not as if people can't click through one additional level to get blame output, and those clickthroughs gradually taper off over time as the code continues to be modified or becomes less important to spelunk back through time.

I suspect my vote won't sway you, but all I can do is try.

PK

[Nico Weber]

(Another issue that I just learned about that makes this difficult is that the Windows code uses NULL heavily in places where no pointer is expected – several of the HANDLE types (not HANDLE itself I believe, but e.g. TRACEHANDLE), and MSDN even says to use NULL for those (http://msdn.microsoft.com/en-us/library/windows/desktop/aa363686(v=vs.85).aspx). This would probably make a mass find-and-replace somewhat involved too.)

[Peter Kasting]

On Wed, Sep 24, 2014 at 2:06 PM, Nico Weber <tha...@chromium.org> wrote:

(Another issue that I just learned about that makes this difficult is that the Windows code uses NULL heavily in places where no pointer is expected – several of the HANDLE types (not HANDLE itself I believe, but e.g. TRACEHANDLE), and MSDN even says to use NULL for those (http://msdn.microsoft.com/en-us/library/windows/desktop/aa363686(v=vs.85).aspx). This would probably make a mass find-and-replace somewhat involved too.)

If we're doing that in our code, that would definitely complicate any mass replacement.  That's a good point.

That said, I suspect the number of real cases we'd break would be small and manageable.  The use of Windows types is significantly more limited than just "all Windows-specific code", and the number of those cases that this applies to is, I hope, small.

PK

[Chris Hopman]

I think I agree with Peter. As we introduce new features, particularly when we are going to say that you should use the new way and not the old way, I think we should prefer to mass rewrite things to use the new way, even if it comes at some cost in churn/blameability. That being said, for cases where the conversion is complicated, then just a having a policy of converting code as it is changed/introduced/etc may be fine.

In this case, maybe we can have a clang plugin/reformatter that identifies places where NULL is actually used as a pointer (like when it is immediately implicitly converted)... and maybe also cases where it is used as a sentinel vararg :). This wouldn't get all cases, but would probably get most while still being reasonable safe.

--

[Adrienne Walker]

"Optimize for consistency and clarity and never for blame" should be a
Chromium style rule.

2014-09-24 14:02 GMT-07:00 'Peter Kasting' via blink-dev
<blin...@chromium.org>:

2014-09-24 14:02 GMT-07:00 'Peter Kasting' via blink-dev
<blin...@chromium.org>:

[Peter Kasting]

On Wed, Sep 24, 2014 at 2:15 PM, Chris Hopman <cjho...@chromium.org> wrote:

In this case, maybe we can have a clang plugin/reformatter that identifies places where NULL is actually used as a pointer (like when it is immediately implicitly converted)... and maybe also cases where it is used as a sentinel vararg :). This wouldn't get all cases, but would probably get most while still being reasonable safe.

Even changing NULL -> nullptr in cross-platform code and leaving Windows devs to update code as they see fit would be OK with me.  I don't want to let perfect be the enemy of good here.

PK

[Chris Hopman]

+1, if our only concerns are the Windows issues nico mentioned, then yeah, that's even better. 

PK

[James Robinson]

If we can do a machine conversion I think we should, but I'm not entirely sure this will be easy.  I don't want to block usage of the feature on their being a good rewriter.  For example, we'll have to reformat many lines of code since nullptr is 3 characters longer which might be tricky in parts of the codebase that aren't currently clang-formatted.

How about we allow nullptr, encourage conversion, and if somebody cooks up a workable translator we allow using it but we do not make that a condition of use of the feature?

- James

On Wed, Sep 24, 2014 at 2:15 PM, Chris Hopman <cjho...@chromium.org> wrote:

[Hendrik]

TRACEHANDLE is ULONG64, so that one won't compile after the replace. Besides, we should probably replace it with =0 instead.  I did a quick look, there looks to only be a few places where it needs to be fixed, so a mass replace, compile, and fix should be fine for this one.

As you mentioned, HANDLE will be fine to use nullptr, as it's just a void*

I suspect the other window handle types would fall into one of these two categories.

[Hendrik]

Lets change the character limit to 83 :)

Just kidding, that's a good point!

On Wednesday, September 24, 2014 2:33:47 PM UTC-7, James Robinson wrote:

If we can do a machine conversion I think we should, but I'm not entirely sure this will be easy.  I don't want to block usage of the feature on their being a good rewriter.  For example, we'll have to reformat many lines of code since nullptr is 3 characters longer which might be tricky in parts of the codebase that aren't currently clang-formatted.

How about we allow nullptr, encourage conversion, and if somebody cooks up a workable translator we allow using it but we do not make that a condition of use of the feature?

- James

On Wed, Sep 24, 2014 at 2:15 PM, Chris Hopman wrote:

I think I agree with Peter. As we introduce new features, particularly when we are going to say that you should use the new way and not the old way, I think we should prefer to mass rewrite things to use the new way, even if it comes at some cost in churn/blameability. That being said, for cases where the conversion is complicated, then just a having a policy of converting code as it is changed/introduced/etc may be fine.

In this case, maybe we can have a clang plugin/reformatter that identifies places where NULL is actually used as a pointer (like when it is immediately implicitly converted)... and maybe also cases where it is used as a sentinel vararg :). This wouldn't get all cases, but would probably get most while still being reasonable safe.

On Wed, Sep 24, 2014 at 2:02 PM, 'Peter Kasting' via Chromium-dev wrote:

[Nico Weber]

On Wed, Sep 24, 2014 at 2:36 PM, Hendrik <hend...@chromium.org> wrote:

TRACEHANDLE is ULONG64, so that one won't compile after the replace. Besides, we should probably replace it with =0 instead.  I did a quick look, there looks to only be a few places where it needs to be fixed, so a mass replace, compile, and fix should be fine for this one.

TRACEHANDLE was just one example, the same is true for various other handle types. I was looking at this for some unrelated reason, and there's hundreds of build warnings about conversions like this ( http://llvm.org/bugs/show_bug.cgi?id=12146#c5 ).

It sounds like everyone but me is in favor of rewriting NULLs to nullptrs, so if someone finds a good way of doing that, we should probably do it. New code can just use it, starting today.

[Peter Kasting]

On Wed, Sep 24, 2014 at 2:33 PM, James Robinson <jam...@chromium.org> wrote:

If we can do a machine conversion I think we should, but I'm not entirely sure this will be easy.  I don't want to block usage of the feature on their being a good rewriter.  For example, we'll have to reformat many lines of code since nullptr is 3 characters longer which might be tricky in parts of the codebase that aren't currently clang-formatted.

clang-format only reformats lines you touch, right?  So presumably using it to reformat these lines would still have minimal impact on the rest of these files (it wouldn't clang-format the whole codebase).

How about we allow nullptr, encourage conversion, and if somebody cooks up a workable translator we allow using it but we do not make that a condition of use of the feature?

I agree that we shouldn't gate on this.  Conversion is a nice-to-have.

My hope was that someone like Daniel who knows enough to do something like the override conversion would be able to handle this as well.

PK 

[Sam McNally]

clang-modernize has a transform for this: http://clang.llvm.org/extra/UseNullptrTransform.html

--

[Peter Kasting]

On Wed, Sep 24, 2014 at 6:01 PM, Sam McNally <sa...@chromium.org> wrote:

clang-modernize has a transform for this: http://clang.llvm.org/extra/UseNullptrTransform.html

Awesome.  Anyone want to volunteer to try and run this on parts of Chromium?

PK 

[Daniel Cheng]

Note the default clang checkout doesn't include the extra clang tools anymore. I added support for it in https://codereview.chromium.org/12213081, but later removed it because no one was using it anymore. Since then, we've moved from make to CMake for building clang so my original patch will likely need to be updated.

Daniel

[Sam McNally]

I gave clang-modernize a try on src/extensions/ and https://codereview.chromium.org/598173003/ is the result.