[clang-tidy] Fix macro-repeated-side-effects false positive on assert - #235410

diff --git a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
index 4306cb11ce4bac..6297e675580559 100644
--- a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp
@@ -12,6 +12,7 @@
 #include "clang/Lex/MacroArgs.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
+#include <optional>
 #include <stack>
 
 namespace clang::tidy::bugprone {
@@ -32,6 +33,8 @@ class MacroRepeatedPPCallbacks : public PPCallbacks {
   unsigned countArgumentExpansions(const MacroInfo *MI,
                                    const IdentifierInfo *Arg) const;
 
+  bool isNonZeroLiteral(const Token &T) const;
+
   bool hasSideEffects(const Token *ResultArgToks) const;
 };
 } // End of anonymous namespace.
@@ -84,10 +87,30 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions(
   // Has a __builtin_constant_p been found?
   bool FoundBuiltin = false;
   bool PrevTokenIsHash = false;
+  int ParenDepth = 0;
+  // Paren depth of the false branch of "(1 ? a : b)" while we are skipping it.
+  std::optional<int> DeadBranchDepth;
   // Count when "?" is reached. The "Current" will get this value when the ":"
-  // is reached.
-  std::stack<unsigned, SmallVector<unsigned, 8>> CountAtQuestion;
-  for (const auto &T : MI->tokens()) {
+  // is reached. Also remembers if the condition was a non-zero literal.
+  std::stack<std::pair<unsigned, bool>,
+             SmallVector<std::pair<unsigned, bool>, 8>>
+      CountAtQuestion;
+  const ArrayRef<Token> Tokens = MI->tokens();
+  for (const auto [I, T] : llvm::enumerate(Tokens)) {
+    if (T.is(tok::l_paren))
+      ParenDepth++;
+    else if (T.is(tok::r_paren))
+      ParenDepth--;
+
+    // The false branch of "(1 ? a : b)" is never evaluated. Skip it until the
+    // closing parenthesis or a comma.
+    if (DeadBranchDepth) {
+      if (ParenDepth > *DeadBranchDepth ||
+          (ParenDepth == *DeadBranchDepth && T.isNot(tok::comma)))
+        continue;
+      DeadBranchDepth.reset();
+    }
+
     // The result of __builtin_constant_p(x) is 0 if x is a macro argument
     // with side effects. If we see a __builtin_constant_p(x) followed by a
     // "?" "&&" or "||", then we need to reason about control flow to report
@@ -108,12 +131,19 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions(
 
     // Handling of ? and :.
     if (T.is(tok::question)) {
-      CountAtQuestion.push(Current);
+      CountAtQuestion.emplace(Current, I >= 2 &&
+                                           Tokens[I - 2].is(tok::l_paren) &&
+                                           isNonZeroLiteral(Tokens[I - 1]));
     } else if (T.is(tok::colon)) {
       if (CountAtQuestion.empty())
         return 0;
-      Current = CountAtQuestion.top();
+      bool SkipFalseBranch = false;
+      std::tie(Current, SkipFalseBranch) = CountAtQuestion.top();
       CountAtQuestion.pop();
+      if (SkipFalseBranch) {
+        DeadBranchDepth = ParenDepth;
+        continue;
+      }
     }
 
     // If current token is a parenthesis, skip it.
@@ -132,6 +162,12 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions(
     if (TII == nullptr)
       continue;
 
+    // Arguments of sizeof and alignof are not evaluated.
+    if (T.isOneOf(tok::kw_sizeof, tok::kw_alignof, tok::kw__Alignof)) {
+      SkipParen = true;
+      continue;
+    }
+
     // If a __builtin_constant_p is found within the macro definition, don't
     // count arguments inside the parentheses and remember that it has been
     // seen in case there are "?", "&&" or "||" operators later.
@@ -159,6 +195,14 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions(
   return Max;
 }
 
+bool MacroRepeatedPPCallbacks::isNonZeroLiteral(const Token &T) const {
+  if (T.isNot(tok::numeric_constant))
+    return false;
+  SmallString<16> Buffer;
+  uint64_t Value = 0;
+  return !PP.getSpelling(T, Buffer).getAsInteger(0, Value) && Value != 0;
+}
+
 bool MacroRepeatedPPCallbacks::hasSideEffects(
     const Token *ResultArgToks) const {
   for (; ResultArgToks->isNot(tok::eof); ++ResultArgToks)
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 037df4cd0937c4..4337ce234db855 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -204,6 +204,12 @@ infrastructure are described first, followed by tool-specific sections.
   <clang-tidy/checks/bugprone/macro-parentheses>` by fixing invalid fixes for
   macro arguments used as names in alias declarations.
 
+- Improved {doc}`bugprone-macro-repeated-side-effects
+  <clang-tidy/checks/bugprone/macro-repeated-side-effects>` check by not
+  counting macro arguments that are never evaluated, such as inside `sizeof`
+  or in the false branch of `(1 ? a : b)`. This fixes false positives on
+  `assert` from glibc 2.44.
+
 - Fixed a crash in {doc}`bugprone-misplaced-operator-in-strlen-in-alloc
   <clang-tidy/checks/bugprone/misplaced-operator-in-strlen-in-alloc>` when
   checking an array new expression without a size expression.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c
index 995d872dce76a0..6657719a49c45b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c
@@ -123,3 +123,28 @@ void log(const char *s, int v);
 void test_log(int a) {
   LOG(a++);
 }
+
+_Bool single_arg(_Bool);
+void fail(const char *);
+#define sizeofgood(x)   (sizeof(x) + _Alignof(x) + (x))
+#define sizeofbad(x)    (sizeof(x) + (x) + (x))
+#define assertc(...)    (((void)sizeof(single_arg(__VA_ARGS__)), __VA_ARGS__) ? (void)0 : fail(#__VA_ARGS__))
+#define assertcpp(...)  ((__VA_ARGS__) ? (void)(1 ? 1 : (_Bool)(__VA_ARGS__)) : fail(#__VA_ARGS__))
+#define deadbad1(x)     ((x) + (0 ? 1 : (x)))
+#define deadbad2(x)     ((x) + (1 ? 1 : (x), (x)))
+void unevaluated(int ret, int a) {
+  ret += sizeofgood(a++);
+  ret += sizeofbad(a++);
+  // CHECK-NOTES: :[[@LINE-1]]:20: warning: side effects in the 1st macro argument 'x'
+  // CHECK-NOTES: :[[@LINE-9]]:9: note: macro 'sizeofbad' defined here
+
+  assertc(++a == 1);
+  assertcpp(++a == 1);
+
+  ret += deadbad1(a++);
+  // CHECK-NOTES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x'
+  // CHECK-NOTES: :[[@LINE-13]]:9: note: macro 'deadbad1' defined here
+  ret += deadbad2(a++);
+  // CHECK-NOTES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x'
+  // CHECK-NOTES: :[[@LINE-15]]:9: note: macro 'deadbad2' defined here
+}