null pointer literals, warnings, and fixups

Clang Frontend
1

It came up as a coding convention question recently on llvm-dev
regarding which null pointer literal to use (NULL or 0) (&
additionally, what was the preferred way to do null pointer tests) &
got me thinking:

What would be the appropriate way to make some warnings (including
fixups) to, primarily, help people migrate to nullptr and,
secondarily, get consistent use of NULL (there are some warnings about
NULL usage in non-pointer contexts (NULL < 3, for example) but not all
(int i = NULL, for example) - and no warning to help you add NULL
where you've currently used 0 as a null pointer literal).

Should these be compiler warnings or static analysis (like the LLVM
coding conventions static analysis)? The last one I mentioned for
C++98 might be a bit tricky. Some people prefer NULL, some prefer 0.
Can we have mutually disagreeing warnings in clang?
The C++0x case seems easier & to be clear what I'm proposing is adding
two warnings: One for any use of NULL that is in a non-pointer
context, with a fixit to switch to the relevant integer literal.
(could this be powered by macro detection semantics (could catch other
definitions of NULL, would fail to catch indirect use of NULL if
someone defined their own MY_NULL macro), or only by GNU's special
__null type?)
And another to uses of NULL in pointer contexts with a fixit to switch
to nullptr.

Side question: what tools currently exist that can actually apply
fixit instructions to code? Is there any existing simple program (that
would probably work like scan-build, or similar) that could be
instructed to actually apply all instances of a particular warning
fixup?

- David

2

<snip>

Side question: what tools currently exist that can actually apply
fixit instructions to code? Is there any existing simple program (that
would probably work like scan-build, or similar) that could be
instructed to actually apply all instances of a particular warning
fixup?

We've been working on infrastructure to do exactly that, but it didn't
get included in mainline (see
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-June/015434.html,
which includes a patch that should still apply somewhat clean). We're
currently working on different options to get it open sourced.

The architectural idea is that you have a clang-fixit tool that uses
cmake's ability to output compile commands to apply all possible fixes
that clang suggests on a file.

Cheers,
/Manuel

3

<snip>

Side question: what tools currently exist that can actually apply
fixit instructions to code? Is there any existing simple program (that
would probably work like scan-build, or similar) that could be
instructed to actually apply all instances of a particular warning
fixup?

We've been working on infrastructure to do exactly that, but it didn't
get included in mainline (see
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-June/015434.html,
which includes a patch that should still apply somewhat clean). We're
currently working on different options to get it open sourced.

Ah, yes, I remember that thread. That's a bit of a bigger/more general
tool than I was thinking of in this context - in this case I was just
wondering if there was a simple tool to apply Clang's suggested
FixIts. In this case if I added a warning that suggested NULL over 0
in pointer contexts (or 0 over NULL, if that was the desired coding
convention) & nullptr over both in C++0x, I would like to be able to
say "apply all fixups from this warning" & then I could easily
transform a codebase over to nullptr (or maintain 0/NULL usage in
C++98).

The architectural idea is that you have a clang-fixit tool that uses
cmake's ability to output compile commands to apply all possible fixes
that clang suggests on a file.

So you'd take all of cmake's output then filter it through some tool
to grab the compile commands, replace the compiler name with your tool
& then your program uses clang as a lib to produce fixits & act on
them? Neat. Though I wonder if it'd be possible/better/worse to use
the clang static analyzer's scan-build approach of injecting itself
into the compilation process & splitting - running both the analyzer
(fixer) & compiler at the same time. Relying on cmake would only work
for people using that build system, scan-build seems to be a little
more versatile.

- David

4

<snip>

Side question: what tools currently exist that can actually apply
fixit instructions to code? Is there any existing simple program (that
would probably work like scan-build, or similar) that could be
instructed to actually apply all instances of a particular warning
fixup?

We've been working on infrastructure to do exactly that, but it didn't
get included in mainline (see
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-June/015434.html,
which includes a patch that should still apply somewhat clean). We're
currently working on different options to get it open sourced.

Ah, yes, I remember that thread. That's a bit of a bigger/more general
tool than I was thinking of in this context - in this case I was just
wondering if there was a simple tool to apply Clang's suggested
FixIts.

That was actually the first use case we developed - a tool that you
can run that applies all fixit-hints.

In this case if I added a warning that suggested NULL over 0
in pointer contexts (or 0 over NULL, if that was the desired coding
convention) & nullptr over both in C++0x, I would like to be able to
say "apply all fixups from this warning" & then I could easily
transform a codebase over to nullptr (or maintain 0/NULL usage in
C++98).

The architectural idea is that you have a clang-fixit tool that uses
cmake's ability to output compile commands to apply all possible fixes
that clang suggests on a file.

So you'd take all of cmake's output then filter it through some tool
to grab the compile commands, replace the compiler name with your tool
& then your program uses clang as a lib to produce fixits & act on
them? Neat.

Close. I implemented a feature in CMake itself to write out all
compile commands into a .json file during generation time. This is
also useful for IDEs (eclipse for example is planning to use that
information for its project generation).
The patch I referred to implements a simple tooling class to which you
hand a frontend action, and the tooling infrastructure reads the .json
file, finds the compile command line for all files you want to run on,
and then runs clang with your specified frontend action over those
files.

Though I wonder if it'd be possible/better/worse to use
the clang static analyzer's scan-build approach of injecting itself
into the compilation process & splitting - running both the analyzer
(fixer) & compiler at the same time. Relying on cmake would only work
for people using that build system, scan-build seems to be a little
more versatile.

Both are valid use cases. Requiring a build is often too expensive
when you want to get really fast feedback to a developer for
incremental small changes (delivering functionality that's close to
what IDEs do for people who use them).
Also, if you want to run the tool many times over the same code, you
can perfectly shard and thus parallelize the tool, whereas the build
would still follow the dependency bottlenecks.
Third, the tool supports virtual file systems, which helps if you want
to run over code that you don't have on a file system.
Forth, you often want more control over the flow when doing tooling -
for example, when implementing a refactoring, you need a source
manager that is only used to apply all the refactorings and is
independent of the TUs that were run, and you need to do this after
all runs finished; also, your tool might want to use a special
diagnostic printer etc.

On the other hand being able to run as a plugin during compile time is
important when you want to annotate the build output with more
information (for example break the build on certain errors, run
analysis during your buildbot compiles, etc). Which is why I think
both are good use cases with different limitations.

Cheers,
/Manuel

5

Have you looked at the -cc1 option "-fixit", which applies Fix-Its in place?

  - Doug

6

That’s precisely what I was looking for - thanks.

So by enabling specific warnings (if my hypothetical “always use nullptr in pointer contexts & always use literal 0/false/etc in non-pointer contexts” fixits were implemented) I could quickly transition an entire codebase to a specific style. Lovely.

Any thoughts on whether such warnings would be suitable? how to handle their overlap with existing more specific warnings & how to handle mutually exclusive warnings (in C++98 land some people might want a warning about using NULL (preferring 0), even in pointer contexts, others might want a warning about 0 in pointer contexts (prefering NULL))?

[curiously - clang -cc1 foo.cpp doesn’t enable colored diagnostics by default (when run on a terminal capable of color - where clang foo.cpp does use color). Is that by design?]

Thanks,

  • David
7

in this case I was just
wondering if there was a simple tool to apply Clang’s suggested
FixIts

Have you looked at the -cc1 option “-fixit”, which applies Fix-Its in place?

That’s precisely what I was looking for - thanks.

So by enabling specific warnings (if my hypothetical “always use nullptr in pointer contexts & always use literal 0/false/etc in non-pointer contexts” fixits were implemented) I could quickly transition an entire codebase to a specific style. Lovely.

Any thoughts on whether such warnings would be suitable? how to handle their overlap with existing more specific warnings & how to handle mutually exclusive warnings (in C++98 land some people might want a warning about using NULL (preferring 0), even in pointer contexts, others might want a warning about 0 in pointer contexts (prefering NULL))?

I don’t think these warnings would be suitable for Clang. Such style-checking transformations belong in a separate tool (e.g., like the ARC migrator).

[curiously - clang -cc1 foo.cpp doesn’t enable colored diagnostics by default (when run on a terminal capable of color - where clang foo.cpp does use color). Is that by design?]

Yes. clang -cc1 is just the internal interface to the compiler. The driver makes all of the interesting decisions, such as detecting whether the terminal supports color.

  • Doug
8

I don’t think these warnings would be suitable for Clang.

Even suggesting nullptr in C++11? With things like the bool warning, NULL* used in non-pointer contexts, etc already, this seems like a fairly logical continuation/generalization of that - or would you say that, even in C++11, that it’s clear that false is probably not intended as a pointer literal but still likely that someone should use 0/NULL/etc rather than nullptr?

Or perhaps these existing warnings are more a case of GCC compatibility, but not really the direction you’d prefer to clang to go in if it weren’t for GCC’s legacy.

  • GCC built the whole __null construct to help catch these issues. Now that we have a null pointer type we don’t even have to do that - we just treat it as the “proper nullptr literal” & warn about anything else (& warn about NULL usage).

[hmm, on the 0/NULL case in C++98 - does clang support the possibility of alternative fixits? (“make this change or this change”) though of course that’d reduce/remove the easy migration benefit in C++98 code]

Such style-checking transformations belong in a separate tool (e.g., like the ARC migrator).

I’ll have to look into how the ARC migrator works - both for the migration & for keeping code style consistent in the long term. That’s part of what I was getting at - I turn on the “use nullptr” warning & then I’m never going to accidentally end up with weird uses of NULL or 0/false/etc in my code (or even in llvm’s code if there was a nice way to implement some C++98 equivalent), ideally.

[curiously - clang -cc1 foo.cpp doesn’t enable colored diagnostics by default (when run on a terminal capable of color - where clang foo.cpp does use color). Is that by design?]

Yes. clang -cc1 is just the internal interface to the compiler. The driver makes all of the interesting decisions, such as detecting whether the terminal supports color.

Makes sense. Thanks,

  • David
9

Do you guys run the ARC migrator on every build? I'm curious if
there's an open source example of how to set up a style checker you
run on every build that's not part of core clang.

10

Even suggesting nullptr in C++11? With things like the bool warning, NULL* used in non-pointer contexts, etc already, this seems like a fairly logical continuation/generalization of that - or would you say that, even in C++11, that it’s clear that false is probably not intended as a pointer literal but still likely that someone should use 0/NULL/etc rather than nullptr?

I suppose the difference (moreso in C++98) is between detecting places where the user was probably way off (false initing pointers, arithmetic comparison with NULL, etc) & a legitimate (if questionable) matter of style.

Recommending nullptr in C++11 seems, to me, to tip towards less a matter of style - even if other null pointers aren’t explicitly deprecated, it’s pretty close to that kind of situation.

I’d have to look through more warnings (clangs & gccs) to see if I could understand/discern some generality about which things should be warnings & which things shouldn’t (hard to tell the latter whether they’ve been omitted by design or just not done yet). Though it’d be nice to build a more general auto-styling tool for things that are clearly outside clang’s scope.

[I wonder how easy it would be to implement line length limits & break in sensible places… seems to be the one LLVM developers most often violate. Then naming conventions (casing), NULL/0 usage (& null-testing expressions, as someone else brought up recently), braced single-statement blocks, early returns, etc… ]

  • David
11

I don't think these warnings would be suitable for Clang. Such
style-checking transformations belong in a separate tool (e.g., like the ARC
migrator).

Do you guys run the ARC migrator on every build?

No. It's a standalone tool that will be run once for a code base.

I'm curious if
there's an open source example of how to set up a style checker you
run on every build that's not part of core clang.

I don't know of any.

  - Doug

12

Do you guys run the ARC migrator on every build?

No. It’s a standalone tool that will be run once for a code base.

Does it work if you run it again - to make sure you haven’t regressed any of its invariants it establishes, or is there some other mechanism for that? (or is it just on the developers to ensure they don’t write non-ARC code?)

  • David
13

It’s tolerant of not-fully-ARC code bases, but it’s not meant to be run on an ongoing basis. The stuff that the ARC migrator tackles is code that was well-formed without ARC but becomes ill-formed when compiling as ARC.

  • Doug
14

I don’t think these warnings would be suitable for Clang.

Even suggesting nullptr in C++11? With things like the bool warning, NULL* used in non-pointer contexts, etc already, this seems like a fairly logical continuation/generalization of that - or would you say that, even in C++11, that it’s clear that false is probably not intended as a pointer literal but still likely that someone should use 0/NULL/etc rather than nullptr?

Just because someone is compiling with C++0x doesn’t mean they want to automatically upgrade their code to C++0x.

Such style-checking transformations belong in a separate tool (e.g., like the ARC migrator).

I’ll have to look into how the ARC migrator works - both for the migration & for keeping code style consistent in the long term. That’s part of what I was getting at - I turn on the “use nullptr” warning & then I’m never going to accidentally end up with weird uses of NULL or 0/false/etc in my code (or even in llvm’s code if there was a nice way to implement some C++98 equivalent), ideally.

I understand what you’re getting at, but I consider it the domain of a separate style checker rather than something that should be implemented as a warning.

  • Doug
15

Just because someone is compiling with C++0x doesn’t mean they want to automatically upgrade their code to C++0x.

Sure - though warnings can be disabled by default and/or by users. It seems like it would be pretty limiting to be entirely agnostic to ‘better’ ways of doing things until the old way is explicitly deprecated by the standard.

Is it reasonable to generalize the existing checks into:

  • using NULL in a non-pointer context (potentially still just leveraging GCC’s __null). Special casing for arithmetic doesn’t catch lots of other places. Is “int i = NULL;” (the more common “char c = NULL;” I suppose - which is arguable, perhaps? but seems to me as wrong as the other cases that already have NULL warnings) any more reasonable than “NULL < 3”?
  • using anything other than 0/NULL/nullptr in a pointer context? Special casing for boolean seems to be a bit overly narrow. What about char zero, for example? Though I suppose 0L might still be in the realm of “things people do intentionally”, so perhaps a blacklist (boolean & char) is still preferred over a whitelist (0, NULL, nullptr)?

I guess the existing warnings came out of GCC compatability so perhaps they need to be preserved as much as possible by name/semantics, which still leaves the “what to do when warnings overlap” question (only relevant if the above is reasonable).

I understand what you’re getting at, but I consider it the domain of a separate style checker rather than something that should be implemented as a warning.

Is this true of the existing NULL/null pointer/false warnings, or is there some distinction between those & any new/modified warnings? (including the limited, but more general ones I’ve mentioned above, leaving aside the issue of wholesale migration to nullptr, NULL, or 0)

  • David
16

Just because someone is compiling with C++0x doesn’t mean they want to automatically upgrade their code to C++0x.

Sure - though warnings can be disabled by default and/or by users.

Off-by-default warnings are not a mechanism to subvert our normal processes for vetting a warning. In general, we should avoid off-by-default warnings: if it’s not good enough to turn on by default, why do we have it at all? The vast majority of users will never see an off-by-default warning.

It seems like it would be pretty limiting to be entirely agnostic to ‘better’ ways of doing things until the old way is explicitly deprecated by the standard.

This is an intentional and desirable limitation. A compiler is not a style checker, nor should it ever be. Now, if the warning is pointing out an actual problem that couldn’t be caught before… that’s something entirely different.

Is it reasonable to generalize the existing checks into:

  • using NULL in a non-pointer context (potentially still just leveraging GCC’s __null). Special casing for arithmetic doesn’t catch lots of other places. Is “int i = NULL;” (the more common “char c = NULL;” I suppose - which is arguable, perhaps? but seems to me as wrong as the other cases that already have NULL warnings) any more reasonable than “NULL < 3”?
  • using anything other than 0/NULL/nullptr in a pointer context? Special casing for boolean seems to be a bit overly narrow. What about char zero, for example? Though I suppose 0L might still be in the realm of “things people do intentionally”, so perhaps a blacklist (boolean & char) is still preferred over a whitelist (0, NULL, nullptr)?

Any integral literal 0 seems like it should be allowed here. Otherwise, it seems entirely reasonable to warn about NULL vs 0 confusion, since this is a source of bugs in practice. The question is whether such a warning will produce too much noise; the only way to figure that out is to implement it and run it across a pile of code.

I guess the existing warnings came out of GCC compatability so perhaps they need to be preserved as much as possible by name/semantics, which still leaves the “what to do when warnings overlap” question (only relevant if the above is reasonable).

GCC compatibility sometimes trumps our rational approach to warnings (and compiler design ). We’re fine with extending the behavior of warnings that GCC also has (GCC changes them from one version to another, too), so long as the extension is good.

I understand what you’re getting at, but I consider it the domain of a separate style checker rather than something that should be implemented as a warning.

Is this true of the existing NULL/null pointer/false warnings, or is there some distinction between those & any new/modified warnings? (including the limited, but more general ones I’ve mentioned above, leaving aside the issue of wholesale migration to nullptr, NULL, or 0)

Warnings are intended to find potential problems in the source code. Style migration is the domain of separate tools.

  • Doug
17

Just because someone is compiling with C++0x doesn’t mean they want to automatically upgrade their code to C++0x.

Sure - though warnings can be disabled by default and/or by users.

Off-by-default warnings are not a mechanism to subvert our normal processes for vetting a warning. In general, we should avoid off-by-default warnings: if it’s not good enough to turn on by default, why do we have it at all? The vast majority of users will never see an off-by-default warning.

Hmm - I can see the concept, but it doesn’t see to match up with my experience/understanding. I tend to compile with everything on (the old incantation with G++ was “-Wall -Wextra -pedantic -ansi”) which provides a whole bunch of things over the baseline. It’s just the case that this usage is really that rare? That most people compile without any warning flags at all?

I suppose I could believe that & most users aren’t nearly so interested in correctness/portability as the language lawyers/academic purists/etc. Though if it’s just a matter of adding a feature that’d be useful for some people & unseen by the man-on-the-street, that doesn’t seem to dismiss the idea entirely. Then the only reason not to do it is cost (which I’m offering) & ongoing cost of having that added complexity in the compiler (which I agree, is something to be cautious about at every turn - I don’t like adding unnecessary surface area if I can help it).

It seems like it would be pretty limiting to be entirely agnostic to ‘better’ ways of doing things until the old way is explicitly deprecated by the standard.

This is an intentional and desirable limitation. A compiler is not a style checker, nor should it ever be. Now, if the warning is pointing out an actual problem that couldn’t be caught before… that’s something entirely different.

Is it reasonable to generalize the existing checks into:

  • using NULL in a non-pointer context (potentially still just leveraging GCC’s __null). Special casing for arithmetic doesn’t catch lots of other places. Is “int i = NULL;” (the more common “char c = NULL;” I suppose - which is arguable, perhaps? but seems to me as wrong as the other cases that already have NULL warnings) any more reasonable than “NULL < 3”?
  • using anything other than 0/NULL/nullptr in a pointer context? Special casing for boolean seems to be a bit overly narrow. What about char zero, for example? Though I suppose 0L might still be in the realm of “things people do intentionally”, so perhaps a blacklist (boolean & char) is still preferred over a whitelist (0, NULL, nullptr)?

Any integral literal 0 seems like it should be allowed here. Otherwise, it seems entirely reasonable to warn about NULL vs 0 confusion, since this is a source of bugs in practice.

Sorry, I’m not quite following you here. Which cases of NULL vs 0 confusion do you think should be caught & which ones should be allowed?

I take it you mean that integer 0 literals should be acceptable as pointer literals (agreed, at least in C++98 - Stroustrup’s argument that it helps show you the reality/ambiguity in your code is sufficient for me to believe/understand that some people do this intentionally. But in C++11 I think it can do better & can help eliminate a source of bugs that use of the literal 0 or NULL can produce that cannot be caught in C++03 code because there is no better feature to describe the author’s intent) but uses of NULL in non-pointer contexts should be caught.

I’d say all uses of NULL in non-pointer contexts should be caught. Technically a C++ implementation is actually free to define NULL as nullptr, so at the very least you have a (theoretical, of course, no current implementation could practically make NULL nullptr today) a portability concern if you use NULL in non-pointer contexts.

[I have to say, I’m kind of surprised no one else on the list has any opinion on this at all - which perhaps indicates that I’m barking up the wrong tree]

The question is whether such a warning will produce too much noise; the only way to figure that out is to implement it and run it across a pile of code.

Fair enough. Is there an appropriate reference set/process that should be used - is this the sort of thing where we would add the warning, on by default, see what the buildbots/etc cough up, or is there some way to replicate all that test coverage locally to avoid randomizing the bots/developers?

I guess the existing warnings came out of GCC compatability so perhaps they need to be preserved as much as possible by name/semantics, which still leaves the “what to do when warnings overlap” question (only relevant if the above is reasonable).

GCC compatibility sometimes trumps our rational approach to warnings (and compiler design ).

Fair enough - I was just wondering whether that was the case here so I could understand which warnings were consistent with your philosophy & which ones were concessions to GCC compatibility which aren’t valid precedent for making similar features/changes that don’t increase GCC compatibility.

We’re fine with extending the behavior of warnings that GCC also has (GCC changes them from one version to another, too), so long as the extension is good.

OK.

I understand what you’re getting at, but I consider it the domain of a separate style checker rather than something that should be implemented as a warning.

Is this true of the existing NULL/null pointer/false warnings, or is there some distinction between those & any new/modified warnings? (including the limited, but more general ones I’ve mentioned above, leaving aside the issue of wholesale migration to nullptr, NULL, or 0)

Warnings are intended to find potential problems in the source code. Style migration is the domain of separate tools.

Right - this comes back to the point about GCC above. I’m trying to understand the philosophy you’re going for so I can make a sound argument in terms of that philosophy. If the existing warnings aren’t consistent with that philosophy then I can’t really draw conclusions about what other features clang should have by looking at them. So if the bool conversion and NULL in arithmetic warnings are ones you think are appropriate, then from that I can try to explain why these other warnings would have similar value/purpose.

Thanks,

  • David
18

Just because someone is compiling with C++0x doesn’t mean they want to automatically upgrade their code to C++0x.

Sure - though warnings can be disabled by default and/or by users.

Off-by-default warnings are not a mechanism to subvert our normal processes for vetting a warning. In general, we should avoid off-by-default warnings: if it’s not good enough to turn on by default, why do we have it at all? The vast majority of users will never see an off-by-default warning.

Hmm - I can see the concept, but it doesn’t see to match up with my experience/understanding. I tend to compile with everything on (the old incantation with G++ was “-Wall -Wextra -pedantic -ansi”) which provides a whole bunch of things over the baseline. It’s just the case that this usage is really that rare? That most people compile without any warning flags at all?

Yes. The vast majority of projects I see compile without changing a single warning flag. And, as Ted’s recent experiments with -Weverything have shown us, warnings that aren’t turned on by default get very little testing coverage and tend to be broken (or break over time).

I suppose I could believe that & most users aren’t nearly so interested in correctness/portability as the language lawyers/academic purists/etc. Though if it’s just a matter of adding a feature that’d be useful for some people & unseen by the man-on-the-street, that doesn’t seem to dismiss the idea entirely. Then the only reason not to do it is cost (which I’m offering) & ongoing cost of having that added complexity in the compiler (which I agree, is something to be cautious about at every turn - I don’t like adding unnecessary surface area if I can help it).

I am very concerned with the ongoing cost, and the accumulated drag on code side and compilation performance from having so many (typically unused) knobs.

It seems like it would be pretty limiting to be entirely agnostic to ‘better’ ways of doing things until the old way is explicitly deprecated by the standard.

This is an intentional and desirable limitation. A compiler is not a style checker, nor should it ever be. Now, if the warning is pointing out an actual problem that couldn’t be caught before… that’s something entirely different.

Is it reasonable to generalize the existing checks into:

  • using NULL in a non-pointer context (potentially still just leveraging GCC’s __null). Special casing for arithmetic doesn’t catch lots of other places. Is “int i = NULL;” (the more common “char c = NULL;” I suppose - which is arguable, perhaps? but seems to me as wrong as the other cases that already have NULL warnings) any more reasonable than “NULL < 3”?
  • using anything other than 0/NULL/nullptr in a pointer context? Special casing for boolean seems to be a bit overly narrow. What about char zero, for example? Though I suppose 0L might still be in the realm of “things people do intentionally”, so perhaps a blacklist (boolean & char) is still preferred over a whitelist (0, NULL, nullptr)?

Any integral literal 0 seems like it should be allowed here. Otherwise, it seems entirely reasonable to warn about NULL vs 0 confusion, since this is a source of bugs in practice.

Sorry, I’m not quite following you here. Which cases of NULL vs 0 confusion do you think should be caught & which ones should be allowed?

I take it you mean that integer 0 literals should be acceptable as pointer literals (agreed, at least in C++98 - Stroustrup’s argument that it helps show you the reality/ambiguity in your code is sufficient for me to believe/understand that some people do this intentionally.

Yes, 0 should be acceptable as a pointer literal, as should 0L. This is convention for a large number of projects and programmers.

But in C++11 I think it can do better & can help eliminate a source of bugs that use of the literal 0 or NULL can produce that cannot be caught in C++03 code because there is no better feature to describe the author’s intent) but uses of NULL in non-pointer contexts should be caught.

You’re assuming that programmers want to migrate all of their code to C++0x and its idioms. Perhaps when C++0x is ubiquitous and the majority of code we see if compiled as C++0x, this will make sense, but that’s certainly not the case now. Perhaps in the future, (long?) after Clang has switched its default dialect to C++0x, we can make this assumption.

For the other side of the equation, check out the warning + Fix-It for this code:

struct Point { int x, y; } p = { x: 10 };

Here, we do have a Fix-It that migrates from (old, non-standard) GNU designated initializer syntax to the C99 syntax. There are two major differences between this warning and a warning that would replace “0” with “nullptr”:

  1. The old syntax is not supported by any standard
  2. Clang defaults to the standard providing the new syntax (C99)
  3. The standard providing the new syntax has been available for a very long time and is widely implemented

I’d say all uses of NULL in non-pointer contexts should be caught.

I’m fine with this.

The question is whether such a warning will produce too much noise; the only way to figure that out is to implement it and run it across a pile of code.

Fair enough. Is there an appropriate reference set/process that should be used - is this the sort of thing where we would add the warning, on by default, see what the buildbots/etc cough up, or is there some way to replicate all that test coverage locally to avoid randomizing the bots/developers?

There’s no reference set, save LLVM and Clang itself. Different people use their own favored projects to see how a warning fares.

  • Doug
19

I completely agree. Is anyone interested in working on a clang plugin that does style checks? That would be hugely useful for a wide variety of purposes!

-Chris

20

Yes, 0 should be acceptable as a pointer literal, as should 0L. This is
convention for a large number of projects and programmers.

Agreed for C++93.

For the C++11 case here’s my attempt at a parallel to existing warnings -Wparentheses. When this warning was added, I assume lots of people would’ve had to change their code (or suppress the warning). It was added to help detect a common source of user error by imposing a new (well, probably partially adopted from some existing developers conventions, I assume), somewhat arbitrary (not a standard construct, just an arbitrary choice of syntax to indicate “I did this deliberately”) syntax to express developer intent to the compiler (& other developers) so the compiler could more accurately detect possible errors.

foo.c:5:9: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  if (a = b)
      ~~^~~

That seems equivalent to suggesting NULL instead of 0 in pointer contexts in C++98, really. If you do that, then the compiler knows what you mean & can help you check that what you mean is what you actually got. (by checking NULL is used always and only in pointer contexts)

The C++11 case of suggesting nullptr seems even stronger than the case for -Wparentheses, in this case nullptr is a language-provided construct designed for the purpose.

But, again, perhaps -Wparentheses fits in the “We’d rather not have this, if it weren’t for GCC’s legacy/compatibility”, since I don’t know which warnings fit & which ones don’t, it’s hard for me to generalize.

You’re assuming that programmers want to migrate all of their code to C++0x and its idioms.

Not entirely - I’m assuming that it’s the ‘right’ way to write C++0x and developers who want to write in some limited subset of C++0x can suppress it. If C++0x has been chosen, it makes sense to me to provide the best diagnostics we can for writing C++0x, not a subset of it. If using nullptr means we can diagnose more pointer mis-usage (indeed, as hard compiler errors, in fact - once you’ve switched to nullptr you won’t just get a silence when you compare a pointer to 0 thinking it was an integer, you’ll get an error) it seems we’d be doing a disservice to C++0x developers not to tell them they were being ambiguous & that the compiler won’t be helping them as much as it could.

It’s probably one of the cheapest (especially with a fixup) migrations to make when compiling code as C++0x. The first 0x feature pretty much any compiler will implement, so I’m not sure many people would choose to compile as C++0x yet resist updating to a construct that helps diagnose bugs and expresses the author intent more clearly (to both other developers and the compiler itself).

Perhaps when C++0x is ubiquitous and the majority of code we see if compiled as C++0x, this will make sense, but that’s certainly not the case now. Perhaps in the future, (long?) after Clang has switched its default dialect to C++0x, we can make this assumption.

It almost makes more sense in a world where C++0x isn’t the default dialect - if a user is opting in to C++0x, it’s likely they’re trying to use its features/benefits and this is one of the low-hanging fruit (if it’s easy to do the migration, which a fixup would provide - check the code compiles, suppress all warnings except the null pointer usage warning and recompile with -fixit).

For the other side of the equation, check out the warning + Fix-It for this code:
struct Point { int x, y; } p = { x: 10 };

I agree, this is a much more compelling warning than the one I’m proposing - and I’m sure there are many other better/less ambiguous warnings than nullptr. My point is that there are many /less/ compelling warnings too (though I don’t know which of those are consistent with your philosophy & which aren’t).

Side note:

/tmp/webcompile/_1251_0.c:1:37: warning: use of GNU old-style field designator extension [-Wgnu-designator]
struct Point { int x, y; } p = { x: 10 };
                                 ~~ ^
                                 .x =

Shouldn’t that ^ be below the x, not the 10? like this:

/tmp/webcompile/_1251_0.c:1:37: warning: use of GNU old-style field designator extension [-Wgnu-designator]
struct Point { int x, y; } p = { x: 10 };
                                 ^~ 
                                 .x =

I’d say all uses of NULL in non-pointer contexts should be caught.

I’m fine with this.

Ok, great! I can do that, I think. Any idea how this would work with the existing -Wnull-arithmetic warning which is a subset of this? (it disallows “NULL + 2” and “NULL < 3” but doesn’t stop “int i = NULL;” (or “void foo(int); foo(NULL)”))

Is a fixit to nullptr applicable here (in C++0x) similar to the diff I provided for the use of ‘false’ in pointer contexts to suggest nullptr in C++0x? Or should the output remain agnostic to null pointer literals in C++11?

The question is whether such a warning will produce too much noise; the
only way to figure that out is to implement it and run it across a pile of
code.

Fair enough. Is there an appropriate reference set/process that should be
used - is this the sort of thing where we would add the warning, on by
default, see what the buildbots/etc cough up, or is there some way to
replicate all that test coverage locally to avoid randomizing the
bots/developers?

There’s no reference set, save LLVM and Clang itself. Different people use
their own favored projects to see how a warning fares.

Ok - though that won’t help with C++0x warnings like I’m suggesting, unfortunately. I wonder if there are any major projects compiling as C++0x currently.

Thanks again for taking the time to help explain all this,

  • David