[cfe-commits] [patch] fixit for null-conversion

David Blaikie dblaikie at gmail.com
Mon Apr 30 10:39:29 PDT 2012


On Mon, Apr 30, 2012 at 9:05 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Apr 29, 2012, at 9:05 PM, David Blaikie wrote:
>
>> Just something I've had lying around for a while - I'm not sure
>> whether this meets the fixit bar, presumably the real mistakes people
>> make when using NULL is that they were calling the wrong function,
>> passing arguments in the wrong order, etc - if they're simply passing
>> NULL when they meant 0, that's not really the cause of bugs.
>
> +const char *Sema::getFixItZeroLiteralForType(QualType T) const {
> +  const char *Str = getFixItZeroInitializerForType(T);
> +  if (!Str)
> +    return Str;
> +  size_t Len = std::strlen(Str);
> +  assert(Len > 3 && "Should be a non-record type init");
> +  assert(strncmp(Str, " = ", 3) == 0 && "Should be non-record type init");
> +  return Str + 3;
> +}
>
> This feels very brittle.

Fair point. (repeating from IRC for posterity - I think I'd wanted to
avoid string concatenation, but since these will all be small that's
probably not worth avoiding. Also, the need to return a "non value"
(null in this case) meant string was a bit awkward - but the use of
the empty string for the non-value has actually tidied some callers up
a bit)

I've refactored it out into a common function - though it means
getFixItZeroLiteralForType is now a trivial wrapper. Should that just
be folded into getFixItZeroLiteralForType, or do you prefer the two to
use a common function to make the sharing a bit clearer?

- David

> At the very least, this needs a comment in getFixItZeroInitializerForType() so that we know to look here each time we change it. getFixItZeroLiteralForType() then needs more assertions so that it is obvious to that future reader what the invariants are. Also, 'Len' isn't needed as a variable, and will cause a warning in a non-asserts build.

[Good points I'll try to keep in mind for future changes - no longer
applicable to this change.]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: null_fixit.diff
Type: application/octet-stream
Size: 6508 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120430/2e9b653a/attachment.obj>


More information about the cfe-commits mailing list