[clang-tidy] readability-use-concise-preprocessor-directives, take consistency into account

[carlosgalvezp]

Consider this example:

#if defined(FOO)  // warning: use #ifdef instead
int x;
#elif defined(BAR)  // but it becomes inconsistent with this one
int y;
#else
int z;
#endif

Here we use if defined because we don't have elifdef until C++23, so we want to keep consistency. However the check warns about it.

Not sure how difficult it would be to implement or if it's worth at all, but would be nice if the check was smart enough to understand this use case (possibly under an option). Otherwise if we think it's too niche of a use case I can just close this and NOLINT it :)

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Carlos Galvez (carlosgalvezp)

Consider this example:

#if defined(FOO)
int x;
elif defined(BAR)
int y;
else
int z;

Here we use if defined because we don't have elifdef until C++23, so we want to keep consistency. However the check warns about it.

Not sure how difficult it would be to implement or if it's worth at all, but would be nice if the check was smart enough to understand this use case (possibly under an option). Otherwise if we think it's too niche of a use case I can just close this and NOLINT it :)

[LegalizeAdulthood]

IMO, this is a false positive

[NagyDonat]

I looked at the code of this check, and ensuring consistency would be a small, but not entirely trivial change. I think the following steps should be sufficient for implementing this:

• When the check finds an #if defined that can be rewritten to #ifdef, put it onto a queue instead of immediately reporting it.
• Run the (already existing) logic for finding potential #elif defined $\to$ #elifdef conversions even in C < 23 where #elifdef is unavailable...
• ...and discard the queued #if defined $\to$ #ifdef conversion when we encounter a potential #elif defined $\to$ #elifdef conversion that cannot be applied because the language version is too old.
• Flush the queue and emit the warnings when we see #endif.
• Put this whole logic behind an option.

I don't have time for actually creating this commit, but I hope that this quick draft can be helpful for someone else (perhaps a new contributor?).

[vbvictor]

CC @localspook

[localspook]

Here’s a slightly different case:

#if defined FOO
#elif defined BAR || defined BAZ
#endif

The #elif cannot be shortened, in any language mode. Do you think the #if should be left alone here as well, to preserve consistency?

How about in this case?

#if defined FOO
#elif CHAR_BIT == 8 || defined BAZ
#endif

[carlosgalvezp]

Good points! For me personally I would say that given an #if - #endif block (including nested #if - #endif), best would be if the whole block had the same style, concise or not. So in your last example I would vote for keeping defined in both branches.

Then comes the question of whole-file consistency, e.g.

#ifdef FOO
#endif

#if defined BAR
#else if defined BAZ
#endif

Some people might argue it's inconsistent that both blocks don't follow the same style. I don't have a very strong opinion, perhaps leaning towards just keeping the whole file consistent. The exception to this rule could be header guards, since it's pretty standard to use the concise form.

This is a very subjective topic so I'm sure there's no right answer, probably best is to have options for people to tweak. But that increases the check's complexity and testing of course, so we need to weight that effort in.

[localspook]

For whole-file consistency, I'm a bit skeptical. I can imagine a large file all using concise style, then someone (pre-C++23) goes and adds one #elif defined(FOO). Suddenly, all the directives in the file need to change to "expanded" style, and the resulting diff has a lot of noise. I imagine many people wouldn't want that tradeoff. (Although if there are people that want it, we can of course implement it.)