Bulk-converting NULL -> nullptr?

[Avi Drissman]

I rejected https://codereview.chromium.org/643823002/ , a mass NULL->nullptr conversion, this morning as churning, based on the line in the C++11 guide that nullptr was for new code only.

In researching this, I went back to our discussion in https://groups.google.com/a/chromium.org/d/msg/chromium-dev/4mijeJHzxLg/0MhUboDhgJsJ and found that I'd missed the decision to investigate using the clang rewriter to do this.

Are we allowing bulk rewrites of NULL -> nullptr? Are we going to try to do it in one sweep using the clang rewriter and therefore the C++11 guide isn't allowing it, or should we allow individuals to take on the task?

Avi

[Avi Drissman]

And we need a decision. The CLs are flooding in:

https://codereview.chromium.org/643823002/

https://codereview.chromium.org/642313003/

https://codereview.chromium.org/640203002/ (landed)

https://codereview.chromium.org/643583003/ (landed)

https://codereview.chromium.org/638353002/

https://codereview.chromium.org/634243003/

https://codereview.chromium.org/649533003/

Avi

[Dana Jansens]

I'd like us to use nullptr consistently. It works with smartpointers so we already want to use it, it'd be nice to not do both. I've been intending to approve CLs to do this in areas I own. 

Thanks for your comments on there about a BUG and the patch descriptions.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Viet-Trung Luu]

A quick look through a couple of changes reveals a number of errors (e.g., dubious changes in comments, and worse in strings). It's really not acceptable to just do a context-free global search-and-replace.

--
--
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.

[Ryan Hamilton]

Completely agree that we should use nullptr consistently. I also think that we can trust OWNERS to do the right thing for CLs in the code they own.

[Matt Giuca]

Regarding comments, is there a preferred way to refer to null values in comments?

As Trung points out, some of these find-replaces make the comments seem a bit weird. For example, in the first CL:

"... if |video_context_provider_| is not NULL" -> "... if |video_context_provider_| is not nullptr"

"The SynchronusCompositorImpl can be NULL if..." -> "The SynchronusCompositorImpl can be nullptr if..."

"Only valid (non-NULL) during..." -> ""Only valid (non-nullptr) during..."

I would suggest that all of these examples would read better if "nullptr" was replaced with "null". I think it's still very clear if comments use the word "null" to refer to the value "nullptr" (in this case, "null" is not an abbreviation for the keyword "nullptr", but an adjective referring to the nullness of the value; in other words "nullptr" is a value that is null).

(Also, my 2c: I don't like the idea of a global find/replace, even if it's done intelligently with a Clang-based tool. Too much churn, and to quote Nico from the other thread, "it makes blame output harder to parse, and lines containing NULL are often tricky and interesting in blame output." But it seems my opinion is outnumbered.)

[Peter Kasting]

I am very strongly in favor of mass-converting this.  Inconsistency here is really confusing.

As to how that's accomplished I don't care, but I definitely don't think we should oppose CLs that do this conversion out of principle.  Opposing them when they get the details of the changes wrong is another matter.

PK 

[Peter Kasting]

On Fri, Oct 10, 2014 at 10:30 AM, Matt Giuca <mgi...@chromium.org> wrote:

Regarding comments, is there a preferred way to refer to null values in comments?

As Trung points out, some of these find-replaces make the comments seem a bit weird. For example, in the first CL:

"... if |video_context_provider_| is not NULL" -> "... if |video_context_provider_| is not nullptr"

"The SynchronusCompositorImpl can be NULL if..." -> "The SynchronusCompositorImpl can be nullptr if..."

"Only valid (non-NULL) during..." -> ""Only valid (non-nullptr) during..."

I would suggest that all of these examples would read better if "nullptr" was replaced with "null". I think it's still very clear if comments use the word "null" to refer to the value "nullptr" (in this case, "null" is not an abbreviation for the keyword "nullptr", but an adjective referring to the nullness of the value; in other words "nullptr" is a value that is null).

I think using "nullptr" in the comments here is fine.  It's at least as good as using "null", IMO probably better.  (I don't like "null" because it's close to both NULL and nullptr, so it looks like some kind of reserved token, but it's not.)  We definitely shouldn't be using NULL in these comments in any case.

PK 

[Dana Jansens]

I think null reads like english, as NULL used to. nullptr is awkward in comments, so I've started asking them to use "null" in comments.

[Peter Kasting]

I guess where we disagree is that I don't think NULL ever read like English.  It always looked like a symbol to me.

I don't dislike null enough that I would object to it.

PK

[James Robinson]

It is an actual English word: https://www.google.com/search?q=define%3Anull

- James

[Viet-Trung Luu]

FWIW, I don't mind the churn and like consistency, but it's bad if the reviewer actually has to go over every part of the change: in that case, it probably would have been less work overall for the reviewer to have made the change her/himself.

Changes in comments aren't obviously correct, and require consideration (even if there's a question of style), even if they don't affect the correctness of the code.

Changes in other contexts (e.g., in strings) require even more consideration, and probably shouldn't be done as a part of a generic NULL -> nullptr conversion.

PK 

[Peter Kasting]

On Fri, Oct 10, 2014 at 10:51 AM, James Robinson <jam...@chromium.org> wrote:

It is an actual English word: https://www.google.com/search?q=define%3Anull

Yes, I am aware of that.  Put it in caps and it looks like you're referring to the C++ well-defined value, not the English word.  Just like if I say OWNERS you know I'm probably referring to the files in our checkout, not just the English word.

The kicker is that such comments actually _were_ referring to the C++ value and not the English word.  So if you're trying to be really pedantic, changing to nullptr is less of a meaning-shift than changing to null.

In any case, this bikeshed has gone on too long; I think Trung's latest message just before mine really captures the important bits here.  If these changes are being done in such a way that they require actual review, that's a Bad Thing.

PK 

[Scott Violet]

+1

[Avi Drissman]

I find myself wanting to rant, after the gem I found in https://codereview.chromium.org/643823002/ :

base::Value::TYPE_NULL -> base::Value::TYPE_nullptr

Ugh.

[Peter Kasting]

On Fri, Oct 10, 2014 at 11:10 AM, Avi Drissman <a...@google.com> wrote:

I find myself wanting to rant, after the gem I found in https://codereview.chromium.org/643823002/ :

base::Value::TYPE_NULL -> base::Value::TYPE_nullptr

Wow.

Maybe the right thing to do here is to write a message to chromium-dev in general saying what the criteria for these changes are.  It's sounding more and more like we'll need to have clang do this transform since doing it by hand is too error-prone.

PK 

[Dana Jansens]

This also happened with a cc enum, though I saw it in review.

[Avi Drissman]

On Fri, Oct 10, 2014 at 2:13 PM, Peter Kasting <pkas...@chromium.org> wrote:

Maybe the right thing to do here is to write a message to chromium-dev in general saying what the criteria for these changes are.

Criteria for these changes? Criteria for any changes:

- If a mass change needs to be done, it needs a BUG to tie all of the CLs together.

- All comments and written descriptions need to be written in grammatical English.

- Changelists sent for review are expected to at least compile.

Am I really setting the bar too high here?

Avi

[Peter Kasting]

No, you're not.  I meant something more specific to this particular arena, though, addressing things like what to do about comments, strings, etc.

PK

[James Robinson]

This seems completely on target.

To the original point, I think mechanical cleanups like this when properly executed are highly valuable to the project since when complete they allow us to eliminate an entire class of bugs by banning raw NULL.  However, many of the NULL->nullptr patches have been poorly executed.  We need to follow proper reviewing procedures and not set a different quality bar for contributions just because it's intended to address a mechanical issue instead of a behavioral one.  NULL->nullptr patches still need reviews and should meet the same quality bar as any other sort of patch.

- James

[Peter Kasting]

On Fri, Oct 10, 2014 at 1:59 PM, Avi Drissman <a...@google.com> wrote:

Did you just volunteer to write up such an email? :)

I'm happy to write an email if there's actually any kind of consensus on these things.  I didn't get the sense that there was.

I'll try and write something anyway and hopefully not step on people's toes.

PK 

[Avi Drissman]

Did you just volunteer to write up such an email? :)

[Brian Manthos]

TLDR: Please reconsider the policy of deferred and comingled modernization of Chromium source code.

---

Greg (grt) recommended that I contribute to this 6 year old topic (apparently awakening it).

Often I find doing code reviews in Edge that there is friction between the desired code result and the available paths to get there.  The relative abundance of NULL in Chromium source is a problem.

An example of this happened yesterday when viewing a proposed change in Edge that introduced a new member pointer.  The initialization was using NULL, which aligned with the rest of the file.

Should I recommend using nullptr for the one introduced variable?  That doesn't seem sane.  Based on a subsequent discussion, I pushed the following CL:

2218560: chrome/installer - update NULL to nullptr | https://chromium-review.googlesource.com/c/chromium/src/+/2218560

Initial feedback:
"I think we generally lean towards making these sort of changes incrementally as code is modified for other reasons rather than en-masse like this."

My gut reaction (which I've had in the past, for example, when reviewing the coding guidelines) was that it's a bit of a cop-out to have guidelines that only apply to new code.  It means you'll have a hybrid codebase forever by trying to making an arbitrary divide between "new code" and "old code" that is an absurd distinction in a large, active codebase.  It also motivates behavior like making an arbitrary change to a file (or renaming it) just so you'll have an excuse to change the "old code" because the presubmit tools now call it "new".  If the new rules have significant value and reasoning, shouldn't they be applied consistently?  Is older code considered less valuable (such that you intentionally let it rot in quality and maintainability)?  Anyway, end of gut reaction.

Regarding "change incrementally as code is modified for other reasons":

1. Functional changes comingled with formatting and "getting up to conventions" is generally bad practice, IMO. Modernization changes should be committed separately from new behavior/features.

2. Comingling means the important part of the change (especially for simple changes) gets buried in a sea of additions and removals when reviewed and in GIT history.

3. Comingling distracts the code review process, and leads to reviewer fatigue which damages the efficiency and completeness of the important part of the review. 4. Comingling presents a problem if the associated functional changes needs to be reverted.

5. Comingling introduces artificial conflicts between unrelated functional changes that happen to touch at least one common source file.

6. The "mixed currency" of a subdirectory, and even worse a source file, is a nightmare for those of us working downstream. When a developer adds a new member variable they have to carefully remember to use different standards for the new code vs. the old code. Changing the old code (comingled or not) introduces merge conflicts that are problematic in the short, medium, and long term. This makes it all the more important to modernize upstream, and the best way to do that is separately so that downstream merges don't "miss" something functional buried in the modernization churn.

Open source code that is in active use really shouldn't be "pro" "mixed convention" codebase. It also shouldn't comingle convention changes with unrelated feature/functional changes. It's friction and generally counterproductive for adoption of the source code.

I'm somewhat surprised that Google policy prefers such chaos.

[Peter Kasting]

On Thu, May 28, 2020 at 12:48 PM 'Brian Manthos' via Chromium-dev <chromi...@chromium.org> wrote:

An example of this happened yesterday when viewing a proposed change in Edge that introduced a new member pointer.  The initialization was using NULL, which aligned with the rest of the file.

Should I recommend using nullptr for the one introduced variable?  That doesn't seem sane.

FWIW, I would say it's OK to do that, but it's a judgment call :).

 

Based on a subsequent discussion, I pushed the following CL:

2218560: chrome/installer - update NULL to nullptr | https://chromium-review.googlesource.com/c/chromium/src/+/2218560

Initial feedback:
"I think we generally lean towards making these sort of changes incrementally as code is modified for other reasons rather than en-masse like this."

I think for this one specifically, the link to https://groups.google.com/a/chromium.org/d/topic/chromium-dev/oQunP1HmIfs/discussion is useful, because it makes it clear that in this case we generally _want_ mass conversions, but were running into a lot of cases where people were doing them wrong (because they literally did string search-and-replace).

So at least for NULL -> nullptr, my takeaway is that an en-masse CL is explicitly desirable.  I'm not sure to what degree I should try to respond to the rest of what you mention, except to say that it looks like the comments here were attempting to ask "what happened to cause you to write this?" (which is a reasonable question for any CL), more than to declare that this is against either an explicit or implicit policy; and I think "incrementally as code as modified" doesn't imply "in the same CL that modifies the code".

To be very clear: we do not have a _policy_ of preferring chaos, frowning on mass changes, etc.  It's reasonable to discuss any/all of individual CLs, particular style changes, and general considerations, so we can make good decisions.  Furthermore, we have repeatedly suggested in the past that people separate cleanup CLs and functional CLs for all the reasons you mention.

PK

[Joshua Bell]

On Thu, May 28, 2020 at 1:21 PM Peter Kasting <pkas...@chromium.org> wrote:

...and I think "incrementally as code as modified" doesn't imply "in the same CL that modifies the code".

Amplifying this!

When realizing that you're dealing with a directory that hasn't been updated to the latest coding style or best practices, it's a Really Good Idea to land an initial CL that modernizes the code, followed by CLs that implement the desired behavior changes.

Separate CLs have many benefits:

• It's easier for the reviewers, because they can put on different mental filters for each CL ("is this a no-op with regard to behavior?" vs. "is this new behavior desirable?")
• It's also simpler for the reviewers to inspect the later behavior changes, since they'll be reviewing modern code. You've made their job even easier!
• It's easier to diagnose any problems. If the bots start complaining or crashes start coming in, it's easier to bisect to just one of the changes - either what should have been a no-op (oops!) or a more scoped change.

Caveat: this can make merges more challenging, but you probably shouldn't be merging a monolithic cleanup+fix CL anyway. So if you're anticipating having to merge a fix to another branch, it's best to flip the order - do the targeted fix, then do the cleanup in later CLs.

 

[Ken Rockot]

On Thu, May 28, 2020 at 12:49 PM 'Brian Manthos' via Chromium-dev <chromi...@chromium.org> wrote:

TLDR: Please reconsider the policy of deferred and comingled modernization of Chromium source code.

---

Greg (grt) recommended that I contribute to this 6 year old topic (apparently awakening it).

Often I find doing code reviews in Edge that there is friction between the desired code result and the available paths to get there.  The relative abundance of NULL in Chromium source is a problem.

An example of this happened yesterday when viewing a proposed change in Edge that introduced a new member pointer.  The initialization was using NULL, which aligned with the rest of the file. 

Should I recommend using nullptr for the one introduced variable?  That doesn't seem sane.  Based on a subsequent discussion, I pushed the following CL:

I think I'd disagree here. It seems fine for the one introduced variable to use nullptr. The inconsistency with surrounding code is mildly offensive but of little practical consequence.

 

2218560: chrome/installer - update NULL to nullptr | https://chromium-review.googlesource.com/c/chromium/src/+/2218560

Initial feedback:
"I think we generally lean towards making these sort of changes incrementally as code is modified for other reasons rather than en-masse like this."

My gut reaction (which I've had in the past, for example, when reviewing the coding guidelines) was that it's a bit of a cop-out to have guidelines that only apply to new code.  It means you'll have a hybrid codebase forever by trying to making an arbitrary divide between "new code" and "old code" that is an absurd distinction in a large, active codebase.  It also motivates behavior like making an arbitrary change to a file (or renaming it) just so you'll have an excuse to change the "old code" because the presubmit tools now call it "new".  If the new rules have significant value and reasoning, shouldn't they be applied consistently?  Is older code considered less valuable (such that you intentionally let it rot in quality and maintainability)?  Anyway, end of gut reaction.

Regarding "change incrementally as code is modified for other reasons":

1. Functional changes comingled with formatting and "getting up to conventions" is generally bad practice, IMO. Modernization changes should be committed separately from new behavior/features. 

2. Comingling means the important part of the change (especially for simple changes) gets buried in a sea of additions and removals when reviewed and in GIT history.

3. Comingling distracts the code review process, and leads to reviewer fatigue which damages the efficiency and completeness of the important part of the review. 4. Comingling presents a problem if the associated functional changes needs to be reverted.

5. Comingling introduces artificial conflicts between unrelated functional changes that happen to touch at least one common source file.

6. The "mixed currency" of a subdirectory, and even worse a source file, is a nightmare for those of us working downstream. When a developer adds a new member variable they have to carefully remember to use different standards for the new code vs. the old code. Changing the old code (comingled or not) introduces merge conflicts that are problematic in the short, medium, and long term. This makes it all the more important to modernize upstream, and the best way to do that is separately so that downstream merges don't "miss" something functional buried in the modernization churn.

I don't think you'll find much opposition to the suggestion that substantial style updates should occur separately from functional changes.

I'm not sure I understand why it's necessarily problematic to have new code follow current style rules even when it's surrounded by old code which does not. In some cases this really matters (e.g. conventions that are viral in nature like Bind vs BindOnce, or the long-done conversion from scoped_ptr to std::unique_ptr), but for something like NULL vs nullptr the cost of local inconsistency seems negligible.

 

Open source code that is in active use really shouldn't be "pro" "mixed convention" codebase. It also shouldn't comingle convention changes with unrelated feature/functional changes. It's friction and generally counterproductive for adoption of the source code.

I'm somewhat surprised that Google policy prefers such chaos.

It's a bit loaded to label this as a preference for chaos. The act of mass conversion itself has potential to create chaos, and there are trade-offs to consider either way. We've made mass changes in the past when the cost of inconsistency was deemed significant enough to justify the churn of mass conversion.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/3d01de38-76d1-4976-929a-a62a276e9e67%40chromium.org.