[clang-tidy] add option to readability-use-concise-preprocessor-directives to take consistency into account

clang-tools-extra/clang-tidy/readability/UseConcisePreprocessorDirectivesCheck.cpp

@@ -11,51 +11,124 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
+#include "llvm/ADT/STLForwardCompat.h"
 
 #include <array>
 
 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

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

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
 ^^^^^^^^^^^^^^
 

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