[clang-tidy] add option to readability-use-concise-preprocessor-directives to take consistency into account
#156220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ctives` to take consistency into account
Member
|
@llvm/pr-subscribers-clang-tools-extra Author: Victor Chernyakin (localspook) ChangesFixes #155680. See that issue for context. The precise heuristic this PR implements is this: none of the conditions in a chain will be shortened if any of them contains the One part of the linked issue this PR doesn't implement is consistency between nested chains: #if defined(FOO) // <--- should not be rewritten.
#if defined(BAR) || defined(BAZ)
#endif
#endifBecause the clang preprocessor skips branches not taken, if Full diff: https://github.com/llvm/llvm-project/pull/156220.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
index 05c0088e6b41b..a8c92fbc619a0 100644
--- a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
@@ -11,6 +11,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/PPCallbacks.h"
#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/STLForwardCompat.h"
#include <array>
@@ -18,44 +19,116 @@ namespace clang::tidy::readability {
namespace {
-class IfPreprocessorCallbacks final : public PPCallbacks {
+enum class DirectiveKind : bool { If, Elif };
+
+struct Rewrite {
+ SourceLocation DirectiveLoc;
+ SourceRange ConditionRange;
+ StringRef Macro;
+ bool Negated;
+ DirectiveKind Directive;
+};
+
+struct StackEntry {
+ SmallVector<Rewrite, 2> Rewrites;
+ bool ApplyingRewritesWouldBreakConsistency = false;
+};
+
+class UseConciseDirectivesPPCallbacks final : public PPCallbacks {
public:
- IfPreprocessorCallbacks(ClangTidyCheck &Check, const Preprocessor &PP)
- : Check(Check), PP(PP) {}
+ UseConciseDirectivesPPCallbacks(UseConcisePreprocessorDirectivesCheck &Check,
+ const Preprocessor &PP,
+ bool PreserveConsistency)
+ : Check(Check), PP(PP), PreserveConsistency(PreserveConsistency) {}
+
+ void Ifdef(SourceLocation, const Token &, const MacroDefinition &) override {
+ Stack.emplace_back();
+ }
+
+ void Ifndef(SourceLocation, const Token &, const MacroDefinition &) override {
+ Stack.emplace_back();
+ }
void If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind) override {
- impl(Loc, ConditionRange, {"ifdef", "ifndef"});
+ Stack.emplace_back();
+ handleCondition(Loc, ConditionRange, DirectiveKind::If);
}
void Elif(SourceLocation Loc, SourceRange ConditionRange, ConditionValueKind,
SourceLocation) override {
- if (PP.getLangOpts().C23 || PP.getLangOpts().CPlusPlus23)
- impl(Loc, ConditionRange, {"elifdef", "elifndef"});
+ handleCondition(Loc, ConditionRange, DirectiveKind::Elif);
+ }
+
+ void Endif(SourceLocation, SourceLocation) override {
+ static constexpr std::array<std::array<StringRef, 2>, 2> Replacements = {{
+ {"ifdef", "ifndef"},
+ {"elifdef", "elifndef"},
+ }};
+
+ const auto &[Rewrites, ApplyingRewritesWouldBreakConsistency] =
+ Stack.back();
+ if (!(PreserveConsistency && ApplyingRewritesWouldBreakConsistency)) {
+ for (const auto &[DirectiveLoc, ConditionRange, Macro, Negated,
+ Directive] : Rewrites) {
+ const StringRef Replacement =
+ Replacements[llvm::to_underlying(Directive)][Negated];
+ Check.diag(
+ DirectiveLoc,
+ "preprocessor condition can be written more concisely using '#%0'")
+ << Replacement
+ << FixItHint::CreateReplacement(DirectiveLoc, Replacement)
+ << FixItHint::CreateReplacement(ConditionRange, Macro);
+ }
+ }
+
+ Stack.pop_back();
}
private:
- void impl(SourceLocation DirectiveLoc, SourceRange ConditionRange,
- const std::array<llvm::StringLiteral, 2> &Replacements) {
+ struct RewriteInfo {
+ StringRef Macro;
+ bool Negated;
+ };
+
+ void handleCondition(SourceLocation DirectiveLoc, SourceRange ConditionRange,
+ DirectiveKind Directive) {
+ if (Directive != DirectiveKind::Elif || PP.getLangOpts().C23 ||
+ PP.getLangOpts().CPlusPlus23)
+ if (const std::optional<RewriteInfo> Rewrite =
+ tryRewrite(ConditionRange)) {
+ Stack.back().Rewrites.push_back({DirectiveLoc, ConditionRange,
+ Rewrite->Macro, Rewrite->Negated,
+ Directive});
+ return;
+ }
+
+ if (!Stack.back().ApplyingRewritesWouldBreakConsistency)
+ Stack.back().ApplyingRewritesWouldBreakConsistency =
+ conditionContainsDefinedOperator(ConditionRange);
+ }
+
+ std::optional<RewriteInfo> tryRewrite(SourceRange ConditionRange) {
// Lexer requires its input range to be null-terminated.
- SmallString<128> Condition =
+ const StringRef SourceText =
Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
PP.getSourceManager(), PP.getLangOpts());
+ SmallString<128> Condition = SourceText;
Condition.push_back('\0');
- Lexer Lex(DirectiveLoc, PP.getLangOpts(), Condition.data(),
- Condition.data(), Condition.data() + Condition.size() - 1);
+ Lexer Lex({}, PP.getLangOpts(), Condition.data(), Condition.data(),
+ Condition.data() + Condition.size() - 1);
Token Tok;
- bool Inverted = false; // The inverted form of #*def is #*ndef.
+ bool Negated = false;
std::size_t ParensNestingDepth = 0;
for (;;) {
if (Lex.LexFromRawLexer(Tok))
- return;
+ return {};
if (Tok.is(tok::TokenKind::exclaim) ||
(PP.getLangOpts().CPlusPlus &&
Tok.is(tok::TokenKind::raw_identifier) &&
Tok.getRawIdentifier() == "not"))
- Inverted = !Inverted;
+ Negated = !Negated;
else if (Tok.is(tok::TokenKind::l_paren))
++ParensNestingDepth;
else
@@ -64,47 +137,80 @@ class IfPreprocessorCallbacks final : public PPCallbacks {
if (Tok.isNot(tok::TokenKind::raw_identifier) ||
Tok.getRawIdentifier() != "defined")
- return;
+ return {};
bool NoMoreTokens = Lex.LexFromRawLexer(Tok);
if (Tok.is(tok::TokenKind::l_paren)) {
if (NoMoreTokens)
- return;
+ return {};
++ParensNestingDepth;
NoMoreTokens = Lex.LexFromRawLexer(Tok);
}
if (Tok.isNot(tok::TokenKind::raw_identifier))
- return;
- const StringRef Macro = Tok.getRawIdentifier();
+ return {};
+
+ // We need a stable StringRef into the ConditionRange, but because Lexer
+ // forces us to work on a temporary null-terminated copy of the
+ // ConditionRange, we need to do this ugly translation.
+ const StringRef Macro = {
+ SourceText.data() + (Tok.getRawIdentifier().data() - Condition.data()),
+ Tok.getRawIdentifier().size()};
while (!NoMoreTokens) {
NoMoreTokens = Lex.LexFromRawLexer(Tok);
if (Tok.isNot(tok::TokenKind::r_paren))
- return;
+ return {};
--ParensNestingDepth;
}
if (ParensNestingDepth != 0)
- return;
-
- Check.diag(
- DirectiveLoc,
- "preprocessor condition can be written more concisely using '#%0'")
- << FixItHint::CreateReplacement(DirectiveLoc, Replacements[Inverted])
- << FixItHint::CreateReplacement(ConditionRange, Macro)
- << Replacements[Inverted];
+ return {};
+
+ return {{Macro, Negated}};
}
- ClangTidyCheck &Check;
+ bool conditionContainsDefinedOperator(SourceRange ConditionRange) {
+ // Lexer requires its input range to be null-terminated.
+ SmallString<128> Condition =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+ Condition.push_back('\0');
+ Lexer Lex({}, PP.getLangOpts(), Condition.data(), Condition.data(),
+ Condition.data() + Condition.size() - 1);
+
+ for (Token Tok;;) {
+ const bool NoMoreTokens = Lex.LexFromRawLexer(Tok);
+ if (Tok.is(tok::TokenKind::raw_identifier) &&
+ Tok.getRawIdentifier() == "defined")
+ return true;
+ if (NoMoreTokens)
+ return false;
+ }
+ }
+
+ UseConcisePreprocessorDirectivesCheck &Check;
const Preprocessor &PP;
+ const bool PreserveConsistency;
+ SmallVector<StackEntry, 4> Stack;
};
} // namespace
+UseConcisePreprocessorDirectivesCheck::UseConcisePreprocessorDirectivesCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ PreserveConsistency(Options.get("PreserveConsistency", false)) {}
+
+void UseConcisePreprocessorDirectivesCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "PreserveConsistency", PreserveConsistency);
+}
+
void UseConcisePreprocessorDirectivesCheck::registerPPCallbacks(
const SourceManager &, Preprocessor *PP, Preprocessor *) {
- PP->addPPCallbacks(std::make_unique<IfPreprocessorCallbacks>(*this, *PP));
+ PP->addPPCallbacks(std::make_unique<UseConciseDirectivesPPCallbacks>(
+ *this, *PP, PreserveConsistency));
}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
index e65b16876a89a..c223987a44b73 100644
--- a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
@@ -21,12 +21,14 @@ namespace clang::tidy::readability {
/// https://clang.llvm.org/extra/clang-tidy/checks/readability/use-concise-preprocessor-directives.html
class UseConcisePreprocessorDirectivesCheck : public ClangTidyCheck {
public:
- using ClangTidyCheck::ClangTidyCheck;
+ UseConcisePreprocessorDirectivesCheck(StringRef Name,
+ ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
- bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
- return true;
- }
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const bool PreserveConsistency;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4fee4f93908da..5a2345877c934 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -243,6 +243,12 @@ Changes in existing checks
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
`IgnoreAliasing`, that allows not looking at underlying types of type aliases.
+- Improved :doc:`readability-use-concise-preprocessor-directives
+ <clang-tidy/checks/readability/use-concise-preprocessor-directives>` check by
+ adding the option `PreserveConsistency`, which prevents shortening
+ directives if it would break visual consistency with other directives in the
+ ``#if`` chain.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
index 30ec7e6b89936..5b9970488d1f7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
@@ -28,3 +28,41 @@ Since C23 and C++23:
#elifdef MEOW
#elifndef MEOW
+
+Options
+-------
+
+.. option:: PreserveConsistency
+
+ If `true`, directives will not be simplified if doing so would break
+ consistency with other directives in a chain. Default is `false`.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+ // Not simplified.
+ #if defined(FOO)
+ #elif defined(BAR) || defined(BAZ)
+ #endif
+
+ // Only simplified in C23 or C++23.
+ #if defined(FOO)
+ #elif defined(BAR)
+ #endif
+
+ // Consistency among *different* chains is not taken into account.
+ #if defined(FOO)
+ #if defined(BAR) || defined(BAZ)
+ #endif
+ #elif defined(HAZ)
+ #endif
+
+ // becomes
+
+ #ifdef FOO
+ #if defined(BAR) || defined(BAZ)
+ #endif
+ #elifdef HAZ
+ #endif
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp
new file mode 100644
index 0000000000000..3959f5b7dd037
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp
@@ -0,0 +1,108 @@
+// RUN: %check_clang_tidy -std=c++98,c++11,c++14,c++17,c++20 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }'
+// RUN: %check_clang_tidy -std=c++23-or-later -check-suffixes=,23 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }'
+
+// RUN: %check_clang_tidy -std=c99,c11,c17 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }' -- -x c
+// RUN: %check_clang_tidy -std=c23-or-later -check-suffixes=,23 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }' -- -x c
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifndef HEADER_GUARD
+#if !defined(HEADER_GUARD)
+#define HEADER_GUARD
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifdef FOO
+#if defined(FOO)
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifndef FOO
+#if !defined(FOO)
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifndef FOO
+#if !defined(FOO)
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined(BAR)
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifdef FOO
+#if defined FOO
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined BAR
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifndef BAZ
+#elif !defined BAZ
+#endif
+
+#ifdef FOO
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined BAR
+#endif
+
+#if ( defined(__cplusplus) && __cplusplus >= 202302L) || \
+ (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L)
+
+
+// Existing code can't decide between concise and verbose form, but
+// we can rewrite it to be consistent.
+//
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifndef FOO
+#if !defined(FOO)
+#elifdef BAR
+#endif
+
+#endif
+
+// Existing code can't decide between concise and verbose form, and rewriting
+// the '#elif defined(BAR)' won't make it more consistent, so leave it alone.
+#ifdef FOO
+#elif defined(BAR)
+#elif defined(BAZ) || defined(HAZ)
+#endif
+
+#if defined(FOO)
+#elif defined(BAR) || defined(BAZ)
+#endif
+
+#if defined(FOO)
+#elif defined(BAR) && BAR == 0xC0FFEE
+#endif
+
+#if defined(FOO)
+#elif BAR == 10 || defined(BAZ)
+#endif
+
+#if FOO
+#elif BAR
+#endif
+
+#if defined FOO && BAR
+#elif defined BAZ
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifdef FOO
+#if defined(FOO)
+#elif BAR
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifdef FOO
+#if defined(FOO)
+#elif 1 == 1
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifndef BAR
+#elif !defined(BAR)
+#endif
+
+#endif // HEADER_GUARD
|
Member
|
@llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) ChangesFixes #155680. See that issue for context. The precise heuristic this PR implements is this: none of the conditions in a chain will be shortened if any of them contains the One part of the linked issue this PR doesn't implement is consistency between nested chains: #if defined(FOO) // <--- should not be rewritten.
#if defined(BAR) || defined(BAZ)
#endif
#endifBecause the clang preprocessor skips branches not taken, if Full diff: https://github.com/llvm/llvm-project/pull/156220.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
index 05c0088e6b41b..a8c92fbc619a0 100644
--- a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp
@@ -11,6 +11,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/PPCallbacks.h"
#include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/STLForwardCompat.h"
#include <array>
@@ -18,44 +19,116 @@ namespace clang::tidy::readability {
namespace {
-class IfPreprocessorCallbacks final : public PPCallbacks {
+enum class DirectiveKind : bool { If, Elif };
+
+struct Rewrite {
+ SourceLocation DirectiveLoc;
+ SourceRange ConditionRange;
+ StringRef Macro;
+ bool Negated;
+ DirectiveKind Directive;
+};
+
+struct StackEntry {
+ SmallVector<Rewrite, 2> Rewrites;
+ bool ApplyingRewritesWouldBreakConsistency = false;
+};
+
+class UseConciseDirectivesPPCallbacks final : public PPCallbacks {
public:
- IfPreprocessorCallbacks(ClangTidyCheck &Check, const Preprocessor &PP)
- : Check(Check), PP(PP) {}
+ UseConciseDirectivesPPCallbacks(UseConcisePreprocessorDirectivesCheck &Check,
+ const Preprocessor &PP,
+ bool PreserveConsistency)
+ : Check(Check), PP(PP), PreserveConsistency(PreserveConsistency) {}
+
+ void Ifdef(SourceLocation, const Token &, const MacroDefinition &) override {
+ Stack.emplace_back();
+ }
+
+ void Ifndef(SourceLocation, const Token &, const MacroDefinition &) override {
+ Stack.emplace_back();
+ }
void If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind) override {
- impl(Loc, ConditionRange, {"ifdef", "ifndef"});
+ Stack.emplace_back();
+ handleCondition(Loc, ConditionRange, DirectiveKind::If);
}
void Elif(SourceLocation Loc, SourceRange ConditionRange, ConditionValueKind,
SourceLocation) override {
- if (PP.getLangOpts().C23 || PP.getLangOpts().CPlusPlus23)
- impl(Loc, ConditionRange, {"elifdef", "elifndef"});
+ handleCondition(Loc, ConditionRange, DirectiveKind::Elif);
+ }
+
+ void Endif(SourceLocation, SourceLocation) override {
+ static constexpr std::array<std::array<StringRef, 2>, 2> Replacements = {{
+ {"ifdef", "ifndef"},
+ {"elifdef", "elifndef"},
+ }};
+
+ const auto &[Rewrites, ApplyingRewritesWouldBreakConsistency] =
+ Stack.back();
+ if (!(PreserveConsistency && ApplyingRewritesWouldBreakConsistency)) {
+ for (const auto &[DirectiveLoc, ConditionRange, Macro, Negated,
+ Directive] : Rewrites) {
+ const StringRef Replacement =
+ Replacements[llvm::to_underlying(Directive)][Negated];
+ Check.diag(
+ DirectiveLoc,
+ "preprocessor condition can be written more concisely using '#%0'")
+ << Replacement
+ << FixItHint::CreateReplacement(DirectiveLoc, Replacement)
+ << FixItHint::CreateReplacement(ConditionRange, Macro);
+ }
+ }
+
+ Stack.pop_back();
}
private:
- void impl(SourceLocation DirectiveLoc, SourceRange ConditionRange,
- const std::array<llvm::StringLiteral, 2> &Replacements) {
+ struct RewriteInfo {
+ StringRef Macro;
+ bool Negated;
+ };
+
+ void handleCondition(SourceLocation DirectiveLoc, SourceRange ConditionRange,
+ DirectiveKind Directive) {
+ if (Directive != DirectiveKind::Elif || PP.getLangOpts().C23 ||
+ PP.getLangOpts().CPlusPlus23)
+ if (const std::optional<RewriteInfo> Rewrite =
+ tryRewrite(ConditionRange)) {
+ Stack.back().Rewrites.push_back({DirectiveLoc, ConditionRange,
+ Rewrite->Macro, Rewrite->Negated,
+ Directive});
+ return;
+ }
+
+ if (!Stack.back().ApplyingRewritesWouldBreakConsistency)
+ Stack.back().ApplyingRewritesWouldBreakConsistency =
+ conditionContainsDefinedOperator(ConditionRange);
+ }
+
+ std::optional<RewriteInfo> tryRewrite(SourceRange ConditionRange) {
// Lexer requires its input range to be null-terminated.
- SmallString<128> Condition =
+ const StringRef SourceText =
Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
PP.getSourceManager(), PP.getLangOpts());
+ SmallString<128> Condition = SourceText;
Condition.push_back('\0');
- Lexer Lex(DirectiveLoc, PP.getLangOpts(), Condition.data(),
- Condition.data(), Condition.data() + Condition.size() - 1);
+ Lexer Lex({}, PP.getLangOpts(), Condition.data(), Condition.data(),
+ Condition.data() + Condition.size() - 1);
Token Tok;
- bool Inverted = false; // The inverted form of #*def is #*ndef.
+ bool Negated = false;
std::size_t ParensNestingDepth = 0;
for (;;) {
if (Lex.LexFromRawLexer(Tok))
- return;
+ return {};
if (Tok.is(tok::TokenKind::exclaim) ||
(PP.getLangOpts().CPlusPlus &&
Tok.is(tok::TokenKind::raw_identifier) &&
Tok.getRawIdentifier() == "not"))
- Inverted = !Inverted;
+ Negated = !Negated;
else if (Tok.is(tok::TokenKind::l_paren))
++ParensNestingDepth;
else
@@ -64,47 +137,80 @@ class IfPreprocessorCallbacks final : public PPCallbacks {
if (Tok.isNot(tok::TokenKind::raw_identifier) ||
Tok.getRawIdentifier() != "defined")
- return;
+ return {};
bool NoMoreTokens = Lex.LexFromRawLexer(Tok);
if (Tok.is(tok::TokenKind::l_paren)) {
if (NoMoreTokens)
- return;
+ return {};
++ParensNestingDepth;
NoMoreTokens = Lex.LexFromRawLexer(Tok);
}
if (Tok.isNot(tok::TokenKind::raw_identifier))
- return;
- const StringRef Macro = Tok.getRawIdentifier();
+ return {};
+
+ // We need a stable StringRef into the ConditionRange, but because Lexer
+ // forces us to work on a temporary null-terminated copy of the
+ // ConditionRange, we need to do this ugly translation.
+ const StringRef Macro = {
+ SourceText.data() + (Tok.getRawIdentifier().data() - Condition.data()),
+ Tok.getRawIdentifier().size()};
while (!NoMoreTokens) {
NoMoreTokens = Lex.LexFromRawLexer(Tok);
if (Tok.isNot(tok::TokenKind::r_paren))
- return;
+ return {};
--ParensNestingDepth;
}
if (ParensNestingDepth != 0)
- return;
-
- Check.diag(
- DirectiveLoc,
- "preprocessor condition can be written more concisely using '#%0'")
- << FixItHint::CreateReplacement(DirectiveLoc, Replacements[Inverted])
- << FixItHint::CreateReplacement(ConditionRange, Macro)
- << Replacements[Inverted];
+ return {};
+
+ return {{Macro, Negated}};
}
- ClangTidyCheck &Check;
+ bool conditionContainsDefinedOperator(SourceRange ConditionRange) {
+ // Lexer requires its input range to be null-terminated.
+ SmallString<128> Condition =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP.getSourceManager(), PP.getLangOpts());
+ Condition.push_back('\0');
+ Lexer Lex({}, PP.getLangOpts(), Condition.data(), Condition.data(),
+ Condition.data() + Condition.size() - 1);
+
+ for (Token Tok;;) {
+ const bool NoMoreTokens = Lex.LexFromRawLexer(Tok);
+ if (Tok.is(tok::TokenKind::raw_identifier) &&
+ Tok.getRawIdentifier() == "defined")
+ return true;
+ if (NoMoreTokens)
+ return false;
+ }
+ }
+
+ UseConcisePreprocessorDirectivesCheck &Check;
const Preprocessor &PP;
+ const bool PreserveConsistency;
+ SmallVector<StackEntry, 4> Stack;
};
} // namespace
+UseConcisePreprocessorDirectivesCheck::UseConcisePreprocessorDirectivesCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ PreserveConsistency(Options.get("PreserveConsistency", false)) {}
+
+void UseConcisePreprocessorDirectivesCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "PreserveConsistency", PreserveConsistency);
+}
+
void UseConcisePreprocessorDirectivesCheck::registerPPCallbacks(
const SourceManager &, Preprocessor *PP, Preprocessor *) {
- PP->addPPCallbacks(std::make_unique<IfPreprocessorCallbacks>(*this, *PP));
+ PP->addPPCallbacks(std::make_unique<UseConciseDirectivesPPCallbacks>(
+ *this, *PP, PreserveConsistency));
}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
index e65b16876a89a..c223987a44b73 100644
--- a/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.h
@@ -21,12 +21,14 @@ namespace clang::tidy::readability {
/// https://clang.llvm.org/extra/clang-tidy/checks/readability/use-concise-preprocessor-directives.html
class UseConcisePreprocessorDirectivesCheck : public ClangTidyCheck {
public:
- using ClangTidyCheck::ClangTidyCheck;
+ UseConcisePreprocessorDirectivesCheck(StringRef Name,
+ ClangTidyContext *Context);
void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
- bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
- return true;
- }
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const bool PreserveConsistency;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4fee4f93908da..5a2345877c934 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -243,6 +243,12 @@ Changes in existing checks
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
`IgnoreAliasing`, that allows not looking at underlying types of type aliases.
+- Improved :doc:`readability-use-concise-preprocessor-directives
+ <clang-tidy/checks/readability/use-concise-preprocessor-directives>` check by
+ adding the option `PreserveConsistency`, which prevents shortening
+ directives if it would break visual consistency with other directives in the
+ ``#if`` chain.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
index 30ec7e6b89936..5b9970488d1f7 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
@@ -28,3 +28,41 @@ Since C23 and C++23:
#elifdef MEOW
#elifndef MEOW
+
+Options
+-------
+
+.. option:: PreserveConsistency
+
+ If `true`, directives will not be simplified if doing so would break
+ consistency with other directives in a chain. Default is `false`.
+
+Example
+^^^^^^^
+
+.. code-block:: c++
+
+ // Not simplified.
+ #if defined(FOO)
+ #elif defined(BAR) || defined(BAZ)
+ #endif
+
+ // Only simplified in C23 or C++23.
+ #if defined(FOO)
+ #elif defined(BAR)
+ #endif
+
+ // Consistency among *different* chains is not taken into account.
+ #if defined(FOO)
+ #if defined(BAR) || defined(BAZ)
+ #endif
+ #elif defined(HAZ)
+ #endif
+
+ // becomes
+
+ #ifdef FOO
+ #if defined(BAR) || defined(BAZ)
+ #endif
+ #elifdef HAZ
+ #endif
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp
new file mode 100644
index 0000000000000..3959f5b7dd037
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-concise-preprocessor-directives-preserve-consistency.cpp
@@ -0,0 +1,108 @@
+// RUN: %check_clang_tidy -std=c++98,c++11,c++14,c++17,c++20 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }'
+// RUN: %check_clang_tidy -std=c++23-or-later -check-suffixes=,23 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }'
+
+// RUN: %check_clang_tidy -std=c99,c11,c17 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }' -- -x c
+// RUN: %check_clang_tidy -std=c23-or-later -check-suffixes=,23 %s readability-use-concise-preprocessor-directives %t -- \
+// RUN: -config='{ CheckOptions: {readability-use-concise-preprocessor-directives.PreserveConsistency: true} }' -- -x c
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifndef HEADER_GUARD
+#if !defined(HEADER_GUARD)
+#define HEADER_GUARD
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifdef FOO
+#if defined(FOO)
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifndef FOO
+#if !defined(FOO)
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifndef FOO
+#if !defined(FOO)
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined(BAR)
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifdef FOO
+#if defined FOO
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined BAR
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifndef BAZ
+#elif !defined BAZ
+#endif
+
+#ifdef FOO
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifdef BAR
+#elif defined BAR
+#endif
+
+#if ( defined(__cplusplus) && __cplusplus >= 202302L) || \
+ (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L)
+
+
+// Existing code can't decide between concise and verbose form, but
+// we can rewrite it to be consistent.
+//
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifndef FOO
+#if !defined(FOO)
+#elifdef BAR
+#endif
+
+#endif
+
+// Existing code can't decide between concise and verbose form, and rewriting
+// the '#elif defined(BAR)' won't make it more consistent, so leave it alone.
+#ifdef FOO
+#elif defined(BAR)
+#elif defined(BAZ) || defined(HAZ)
+#endif
+
+#if defined(FOO)
+#elif defined(BAR) || defined(BAZ)
+#endif
+
+#if defined(FOO)
+#elif defined(BAR) && BAR == 0xC0FFEE
+#endif
+
+#if defined(FOO)
+#elif BAR == 10 || defined(BAZ)
+#endif
+
+#if FOO
+#elif BAR
+#endif
+
+#if defined FOO && BAR
+#elif defined BAZ
+#endif
+
+// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES: #ifdef FOO
+#if defined(FOO)
+#elif BAR
+#endif
+
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#ifdef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #ifdef FOO
+#if defined(FOO)
+#elif 1 == 1
+// CHECK-MESSAGES-23: :[[@LINE+2]]:2: warning: preprocessor condition can be written more concisely using '#elifndef' [readability-use-concise-preprocessor-directives]
+// CHECK-FIXES-23: #elifndef BAR
+#elif !defined(BAR)
+#endif
+
+#endif // HEADER_GUARD
|
localspook
requested review from
5chmidti,
EugeneZelenko,
HerrCai0907,
PiotrZSL,
carlosgalvezp and
vbvictor
vbvictor
reviewed
Sep 10, 2025
| namespace { | ||
|
|
||
| class IfPreprocessorCallbacks final : public PPCallbacks { | ||
| enum class DirectiveKind : bool { If, Elif }; |
Contributor
There was a problem hiding this comment.
If we use enum class, does it matter to specify a type if it wouldn't be converted-to anyway?
Contributor
Author
There was a problem hiding this comment.
The idea is to make the type as small as possible (just like with performance-enum-size)
Contributor
There was a problem hiding this comment.
I guess we are splitting hairs at this point, but why is this important? IMO this is premature optimization.
Contributor
Author
There was a problem hiding this comment.
This is just something I've gotten into the habit of doing by default. I don't like doing it, I think the underlying type being int unless otherwise specified is one of C++'s bad defaults, but the cost of fighting that default is small.
clang-tools-extra/docs/clang-tidy/checks/readability/use-concise-preprocessor-directives.rst
Outdated
Show resolved
Hide resolved
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vbvictor
reviewed
Nov 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #155680.
The precise heuristic this PR implements is this: none of the conditions in a chain will be shortened if any of them contains the
definedoperator but cannot be shortened. See the added tests for how that heuristic works in practice.One part of the linked issue this PR doesn't implement is consistency between nested chains:
Because the clang preprocessor skips branches not taken, if
defined(FOO)is false, the check never sees the innerdefined(BAR) || defined(BAZ):(