r347339 - [clang][Parse] Diagnose useless null statements / empty init-statements

Clang Frontend
#1

Hi Roman,

I’m against this new warning.

A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')

As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Regards,
George

#2

Hi Roman,

I’m against this new warning.

A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')

Yeah, that's not good.

As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Would you be okay with the warning if it was only diagnosed when the
source location of the semi-colon is not immediately preceded by a
macro expansion location? e.g.,

EMPTY_EXPANSION(12); // Not diagnosed
EMPTY_EXPANSION; // Not diagnosed
; // Diagnosed

~Aaron

#3

Hi Roman,

I’m against this new warning.

A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')

Yeah, that's not good.

As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Would you be okay with the warning if it was only diagnosed when the
source location of the semi-colon is not immediately preceded by a
macro expansion location? e.g.,

EMPTY_EXPANSION(12); // Not diagnosed
EMPTY_EXPANSION; // Not diagnosed
; // Diagnosed

This could work provided that all empty space preceding the semicolon is ignored (as the macro could be separated by space(s) or newline(s).
I’m not sure the complexity would be worth it, as I haven’t seen bugs arising from extra semicolons and empty statements,
but it would take care of my use case.

#4

Hi Roman,

I’m against this new warning.

A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')

Yeah, that's not good.

As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Would you be okay with the warning if it was only diagnosed when the
source location of the semi-colon is not immediately preceded by a
macro expansion location? e.g.,

EMPTY_EXPANSION(12); // Not diagnosed
EMPTY_EXPANSION; // Not diagnosed
; // Diagnosed

This could work provided that all empty space preceding the semicolon is ignored (as the macro could be separated by space(s) or newline(s).
I’m not sure the complexity would be worth it, as I haven’t seen bugs arising from extra semicolons and empty statements,
but it would take care of my use case.

Or rather when the *previous* semicolon is coming from a macro.

#5

>
>>
>>> Hi Roman,

Hi.

>>> I’m against this new warning.
>>>
>>> A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
>>> The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')
>> Yeah, that's not good.

It does warn on such cases when the macro not expanded to nothing, yes.

>>> As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.

I don't disagree.
The alternative solution would be to try to squeeze this bit of info
into the AST,
without increasing the size of the particular type, and then emit the
diag in clang-tidy.
I did consider it, it seemed like a over-engineering.

>>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

>> Would you be okay with the warning if it was only diagnosed when the
>> source location of the semi-colon is not immediately preceded by a
>> macro expansion location? e.g.,
>>
>> EMPTY_EXPANSION(12); // Not diagnosed
>> EMPTY_EXPANSION; // Not diagnosed
>> ; // Diagnosed
> This could work provided that all empty space preceding the semicolon is ignored (as the macro could be separated by space(s) or newline(s).
> I’m not sure the complexity would be worth it, as I haven’t seen bugs arising from extra semicolons and empty statements,
> but it would take care of my use case.
>

Or rather when the *previous* semicolon is coming from a macro.

I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.
So at most i would be ok with emitting the macro case under -W{flag}-macro.
Thoughts?

>> ~Aaron
>>
>>> Regards,
>>> George

Roman.

#6

>
>
>
> >
> >>
> >>> Hi Roman,
Hi.

> >>> I’m against this new warning.
> >>>
> >>> A very common idiom is defining a “DEBUG” macro to be a no-op in release, and a logging function otherwise.
> >>> The new introduced warning warns on all such cases (e.g. 'DEBUG(“mystring”);')
> >> Yeah, that's not good.
It does warn on such cases when the macro not expanded to nothing, yes.

> >>> As noted in the review, Clang warnings should generally not be used to diagnose style issues — and an extra semicolon does not seem to be a common bug source.
I don't disagree.
The alternative solution would be to try to squeeze this bit of info
into the AST,
without increasing the size of the particular type, and then emit the
diag in clang-tidy.
I did consider it, it seemed like a over-engineering.

> >>> It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.
Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn't supposed to be used like that (admittedly, i
*do* use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

> >> Would you be okay with the warning if it was only diagnosed when the
> >> source location of the semi-colon is not immediately preceded by a
> >> macro expansion location? e.g.,
> >>
> >> EMPTY_EXPANSION(12); // Not diagnosed
> >> EMPTY_EXPANSION; // Not diagnosed
> >> ; // Diagnosed
> > This could work provided that all empty space preceding the semicolon is ignored (as the macro could be separated by space(s) or newline(s).
> > I’m not sure the complexity would be worth it, as I haven’t seen bugs arising from extra semicolons and empty statements,
> > but it would take care of my use case.
> >
>
> Or rather when the *previous* semicolon is coming from a macro.
I don't like this. clang is already wa-a-ay too forgiving about
macros, it almost ignores almost every warning in macros.
It was an *intentional* decision to still warn on useless ';' after
non-empty macros.

I think I'm confused now. This is a test case:

NULLMACRO(v7); // OK. This could be either GOODMACRO() or
BETTERMACRO() situation, so we can't know we can drop it.

So empty macro expansions should be ignored as I expected... so what
is being diagnosed? George, can you provide a more clear example that
demonstrates the issue?

To be clear, I do *not* think this is a situation we should spend
effort supporting (assuming "all available warnings" really means
-Weverything and not something else):

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

However, if we are diagnosing *empty* macro expansions followed by a
semi-colon, I think that would be unacceptably chatty and is worth
fixing on its own merits.

~Aaron

#7

These are the cases I get:

#define unexpected(a) { kprintf(“unexpected %s:%d\n”, FILE, LINE); a; }

and

#if 0
#define DEBG(fmt, args…) { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args…) {}
#endif

Now calls

DEBG(‘x’); and unexpected(‘msg’);

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

It is a problem in practice for us since we have projects which enable all available warnings and also enable “-Werror”.

Hm, they have asked for -Weverything and they got it. Seems to be
working as intended.
When in previous discussions elsewhere i have talked about
-Weverything, i was always
been told that it isn’t supposed to be used like that (admittedly, i
do use it like that :)).

Could you explain how is this different from any other warning that is
considered pointless by the project?
Why not simply disable it?

You are right, it could be disabled. It’s just for this warning the noise vs. usefulness ratio seemed pretty low.

#8

These are the cases I get:

#define unexpected(a) { kprintf(“unexpected %s:%d\n”, FILE, LINE); a; }

and

#if 0
#define DEBG(fmt, args…) { IOLog(fmt, ## args); kprintf(fmt, ## args); }
#else
#define DEBG(fmt, args…) {}
#endif

Now calls

DEBG(‘x’); and unexpected(‘msg’);

cause the warning to be fired.
Of course, semicolon could be omitted, but the fact that the macro has braces is sort of an implementation detail.

Greg Parker has pointed out it’s possible to rewrite the macros using the “do { … } while (0)”,
but that is a lot of macros to change, and the resulting code would be arguably less readable.

That suggestion is the (or at least an) idiomatic way to write a “statement-like” macro that expects a semicolon. The approach taken by the above macros is often considered to be a macro antipattern (try putting it in an unbraced if and adding an else clause, for example).

#9

We have about 100k unique occurrences of this warning across a slice of our software stack. It’s simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple’s clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that’s sometimes caught. This warning doesn’t seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to “modernize” if they want to.

Cheers,
Alex

#10

We have about 100k unique occurrences of this warning across a slice of our software stack. It's simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple's clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that's sometimes caught. This warning doesn't seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to "modernize" if they want to.

I'm not certain I agree that this does not catch unintended bugs.
After all, it started a discussion that pointed out design issues with
two different macros which were provided as examples of
false-positives. I think this diagnostic points out bad code smells
and is no more stylistic than some of the other diagnostics in
-Weverything, like not having a previous prototype before defining a
function. I am not sold on the idea that this diagnostic should be
moved into clang-tidy simply because it's showing up in a lot of
-Weverything builds.

~Aaron

#11

Nobody should be using “-Weverything” in their default build flags! It should only be used as a way to find the names of interesting warning flags. I’d note that “-Weverything” even has warnings that conflict with each-other…

It’s unworkable for clang to have a policy which prevents the addition of new warning options just because someone enables -Weverything… If people feel like that is what’s happening now, we need to fix that. If that’s what it comes to, even deleting the -Weverything flag would be better.

The question I’d ask for new warnings is a different one: is this warning something which people could feasibly decide to opt into, with -Werror, for their entire project? This does appear to meet that criteria – the cases where it warns do appear to be indicating potential issues, and the code fix which silences the warning will improve the code, not make it worse.

#12

We have about 100k unique occurrences of this warning across a slice of our software stack. It’s simply not feasible to us to adopt it, so we would prefer for it to be reverted to avoid downstream divergence in Apple’s clang. Granted, these occur in projects that use -Weverything but those projects are typically understanding of warnings that they can turn off when they see that they provide useful value in terms of warnings about unintended behavior that’s sometimes caught. This warning doesn’t seem to catch unintended bugs and it would be a heroic battle to adopt it for us in a way where projects are somewhat satisfied. It seems to be a better fit for a clang-tidy check that could suggest automatic fixits and that the users could run on their codebase to “modernize” if they want to.

Please refer to the review thread where -Weverything was added:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20110815/045292.html

Users using -Weverything have always been expected to turn off warnings that they find not useful for their use cases (or to not use -Weverything if they cannot tolerate new warnings) – in fact, the very model for -Weverything is that you see new warnings and turn off the ones you don’t like. That’s the model that users explicitly opt into when they ask for -Weveryhing. It would be harmful to allow only lowest-common-denominator warnings in order to avoid breaking people who incautiously opted into -Weverything -Werror, so the fact that a disabled-by-default warning is problematic for such users is not a compelling reason to remove the warning.

If the warning itself does not objectively have merit, then we can remove it, but the fact that it breaks -Weverything -Werror builds shouldn’t be a consideration.