[cfe-commits] r152940 - in /cfe/trunk: lib/Sema/SemaChecking.cpp test/SemaCXX/conversion.cpp

[David Blaikie]

On Fri, Mar 16, 2012 at 3:38 PM, Chandler Carruth <chandlerc at google.com> wrote:
> David, I don't think is really the right approach...
>
> On Fri, Mar 16, 2012 at 1:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Suppress macro expansion of NULL in NULL warnings.
>
>
> This only suppresses it in one warning though, not in all warnings. I don't
> see why we don't want to always suppress the NULL macro expansion.

Nor do I - the implementation of the NULL macro is an opaque
implementation detail.

> I'd be
> interested in moving this into the logic for generating macro expansion
> notes themselves.

Hmm, OK.

> We could potentially generalize it to other standard-defined macros if beneficial.

There's at least one other instance where this same technique is
already used in some Objective C code:

http://llvm.org/viewvc/llvm-project?view=rev&revision=140711
(<rant>change made without tests</rant>)

>
>> For "int i = NULL;" we would produce:
>>
>> null.cpp:5:11: warning: implicit conversion of NULL constant to integer
>> [-Wconversion]
>>  int i = NULL;
>>      ~   ^~~~
>> null.cpp:1:14: note: expanded from macro 'NULL'
>> \#define NULL __null
>>              ^~~~~~
>>
>> But we really shouldn't trace that macro expansion back into the header,
>> yet we
>> still want macro back traces for code like this:
>>
>> \#define FOO NULL
>> int i = FOO;
>>
>> or
>>
>> \#define FOO int i = NULL;
>> FOO
>
>
> What about the case I'm more worried about:
>
> #undef NULL
> #define NULL FOO

That would be non-conforming (I'm not saying we can dismiss off hand
anything non-conforming, mind you). How much do you think this kind of
thing would occur in practice & be problematic to this diagnostic
approach?

As long as "__null" doesn't appear in the code, this diagnostic won't
fire. Redefining NULL to be something other than __null will just
suppress the warning without any real hope of us recovering (unless we
want to get fancier & inspect the spelling of the null literal in case
someone #def's their NULL to 0 instead of using the standard
definition - but I assume we shouldn't be dabbling with such usage -
if it's not the real NULL macro, we shouldn't care).

The risk would be that the user writes __null in their own custom macro:

#define FOO int i = __null;

Now we won't trace into FOO in the diagnostic. Checking the spelling
of the macro could address this - but do we care about users using
__null directly?

> We need to predicate this on the actual contents of the NULL macro.

As outlined above, it's sort of the opposite - predicate it on the
macro name used to stamp out __null, potentially.

> Also, doing the checks you're already doing, much less my suggested extra
> checking, shouldn't be done if the diagnostic is suppressed for any reason,
> which would also argue for sinking it into the macro expansion layer.

Perhaps this is pedantic, maybe it's practical - but assuming that
when we see __null that it's the real implementation-provided NULL is
correct, but assuming that the NULL macro, regardless of definition,
is the implementation-provided one is incorrect (if the user includes
no system headers they're allowed to define their own NULL). If we
built this into the diagnostic engine, would we be able to make that
distinction? Or would we just not bother?

- David