[clang-tidy] Add support for struct initialization in bugprone-argument-comment

[zeyi2]

Closes #170921

[zeyi2]

This is not a very small PR so I think it would be easier to review commit-by-commit.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code linter.

[vbvictor]

This is not a very small PR so I think it would be easier to review commit-by-commit.

For me, it's hard to review per-commit in github UI.
I'd more like to first have a couple of NFC refactor PRs that we can accept easily, and then have a final functional PR.

The idea of stacked PRs can be applied here, but I can't truly recommend because I still don't know how to make them properly

[localspook]

For me, it's hard to review per-commit in github UI.

Would you be willing to talk about what makes it difficult? (I've now made a couple of PRs with that review style in mind, so I'm wondering whether I could do something differently, like the stacking you mention, to make it easier).

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: mitchell (zeyi2)

Changes

Closes #170921

---

Patch is 21.97 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/171757.diff

5 Files Affected:

• (modified) clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp (+245-57)
• (modified) clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h (+5)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
• (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst (+15)
• (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp (+132)

diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
index ed30d01e645d1..7ae84003655bf 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -75,6 +75,7 @@ void ArgumentCommentCheck::registerMatchers(MatchFinder *Finder) {
                                           isFromStdNamespaceOrSystemHeader())))
                          .bind("expr"),
                      this);
+  Finder->addMatcher(initListExpr().bind("expr"), this);
 }
 
 static std::vector<std::pair<SourceLocation, StringRef>>
@@ -142,31 +143,46 @@ getCommentsBeforeLoc(ASTContext *Ctx, SourceLocation Loc) {
   return Comments;
 }
 
-static bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
-                         StringRef ArgName, unsigned ArgIndex) {
-  const std::string ArgNameLowerStr = ArgName.lower();
-  const StringRef ArgNameLower = ArgNameLowerStr;
+template <typename NamedDeclRange>
+static bool isLikelyTypo(const NamedDeclRange &Candidates, StringRef ArgName,
+                         StringRef TargetName) {
+  llvm::SmallString<64> ArgNameLower;
+  ArgNameLower.reserve(ArgName.size());
+  for (const char C : ArgName)
+    ArgNameLower.push_back(llvm::toLower(C));
+  const StringRef ArgNameLowerRef = StringRef(ArgNameLower);
   // The threshold is arbitrary.
   const unsigned UpperBound = ((ArgName.size() + 2) / 3) + 1;
-  const unsigned ThisED = ArgNameLower.edit_distance(
-      Params[ArgIndex]->getIdentifier()->getName().lower(),
-      /*AllowReplacements=*/true, UpperBound);
+  llvm::SmallString<64> TargetNameLower;
+  TargetNameLower.reserve(TargetName.size());
+  for (const char C : TargetName)
+    TargetNameLower.push_back(llvm::toLower(C));
+  const unsigned ThisED =
+      ArgNameLowerRef.edit_distance(StringRef(TargetNameLower),
+                                    /*AllowReplacements=*/true, UpperBound);
   if (ThisED >= UpperBound)
     return false;
 
-  for (unsigned I = 0, E = Params.size(); I != E; ++I) {
-    if (I == ArgIndex)
-      continue;
-    const IdentifierInfo *II = Params[I]->getIdentifier();
+  for (const auto &Candidate : Candidates) {
+    const IdentifierInfo *II = Candidate->getIdentifier();
     if (!II)
       continue;
 
+    // Skip the target itself.
+    if (II->getName() == TargetName)
+      continue;
+
     const unsigned Threshold = 2;
-    // Other parameters must be an edit distance at least Threshold more away
-    // from this parameter. This gives us greater confidence that this is a
-    // typo of this parameter and not one with a similar name.
-    const unsigned OtherED = ArgNameLower.edit_distance(
-        II->getName().lower(),
+    // Other candidates must be an edit distance at least Threshold more away
+    // from this candidate. This gives us greater confidence that this is a
+    // typo of this candidate and not one with a similar name.
+    llvm::SmallString<64> CandidateLower;
+    const StringRef CandName = II->getName();
+    CandidateLower.reserve(CandName.size());
+    for (const char C : CandName)
+      CandidateLower.push_back(llvm::toLower(C));
+    const unsigned OtherED = ArgNameLowerRef.edit_distance(
+        StringRef(CandidateLower),
         /*AllowReplacements=*/true, ThisED + Threshold);
     if (OtherED < ThisED + Threshold)
       return false;
@@ -243,6 +259,63 @@ static const FunctionDecl *resolveMocks(const FunctionDecl *Func) {
   return Func;
 }
 
+// Create a file character range between two locations, or return an invalid
+// range if they are from different files or otherwise not representable.
+static CharSourceRange makeFileCharRange(ASTContext *Ctx, SourceLocation Begin,
+                                         SourceLocation End) {
+  return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End),
+                                  Ctx->getSourceManager(), Ctx->getLangOpts());
+}
+
+// Collects consecutive comments immediately preceding an argument expression.
+static llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4>
+collectLeadingComments(ASTContext *Ctx, SourceLocation SearchBegin,
+                       const Expr *Arg) {
+  const CharSourceRange BeforeArgument =
+      makeFileCharRange(Ctx, SearchBegin, Arg->getBeginLoc());
+
+  if (BeforeArgument.isValid()) {
+    auto Vec = getCommentsInRange(Ctx, BeforeArgument);
+    llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Result;
+    Result.append(Vec.begin(), Vec.end());
+    return Result;
+  }
+
+  // Fall back to parsing back from the start of the argument.
+  const CharSourceRange ArgsRange =
+      makeFileCharRange(Ctx, Arg->getBeginLoc(), Arg->getEndLoc());
+  auto Vec = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin());
+  llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Result;
+  Result.append(Vec.begin(), Vec.end());
+  return Result;
+}
+
+template <typename NamedDeclRange>
+static bool diagnoseMismatchInComment(
+    ArgumentCommentCheck &Self, ASTContext *Ctx,
+    const NamedDeclRange &Candidates, const IdentifierInfo *II, bool StrictMode,
+    llvm::Regex &IdentRE, const std::pair<SourceLocation, StringRef> &Comment,
+    SourceLocation DeclLoc) {
+  llvm::SmallVector<StringRef, 2> Matches;
+  if (!(IdentRE.match(Comment.second, &Matches) &&
+        !sameName(Matches[2], II->getName(), StrictMode)))
+    return false;
+
+  {
+    const DiagnosticBuilder Diag =
+        Self.diag(
+            Comment.first,
+            "argument name '%0' in comment does not match parameter name %1")
+        << Matches[2] << II;
+    if (isLikelyTypo(Candidates, Matches[2], II->getName())) {
+      Diag << FixItHint::CreateReplacement(
+          Comment.first, (Matches[1] + II->getName() + Matches[3]).str());
+    }
+  }
+  Self.diag(DeclLoc, "%0 declared here", DiagnosticIDs::Note) << II;
+  return true;
+}
+
 // Given the argument type and the options determine if we should
 // be adding an argument comment.
 bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const {
@@ -274,12 +347,6 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
   if ((NumArgs == 0) || (IgnoreSingleArgument && NumArgs == 1))
     return;
 
-  auto MakeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) {
-    return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End),
-                                    Ctx->getSourceManager(),
-                                    Ctx->getLangOpts());
-  };
-
   for (unsigned I = 0; I < NumArgs; ++I) {
     const ParmVarDecl *PVD = Callee->getParamDecl(I);
     const IdentifierInfo *II = PVD->getIdentifier();
@@ -296,47 +363,25 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
       }
     }
 
-    const CharSourceRange BeforeArgument =
-        MakeFileCharRange(ArgBeginLoc, Args[I]->getBeginLoc());
+    const llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Comments =
+        collectLeadingComments(Ctx, ArgBeginLoc, Args[I]);
     ArgBeginLoc = Args[I]->getEndLoc();
 
-    std::vector<std::pair<SourceLocation, StringRef>> Comments;
-    if (BeforeArgument.isValid()) {
-      Comments = getCommentsInRange(Ctx, BeforeArgument);
-    } else {
-      // Fall back to parsing back from the start of the argument.
-      const CharSourceRange ArgsRange =
-          MakeFileCharRange(Args[I]->getBeginLoc(), Args[I]->getEndLoc());
-      Comments = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin());
-    }
-
     for (auto Comment : Comments) {
-      llvm::SmallVector<StringRef, 2> Matches;
-      if (IdentRE.match(Comment.second, &Matches) &&
-          !sameName(Matches[2], II->getName(), StrictMode)) {
-        {
-          const DiagnosticBuilder Diag =
-              diag(Comment.first, "argument name '%0' in comment does not "
-                                  "match parameter name %1")
-              << Matches[2] << II;
-          if (isLikelyTypo(Callee->parameters(), Matches[2], I)) {
-            Diag << FixItHint::CreateReplacement(
-                Comment.first, (Matches[1] + II->getName() + Matches[3]).str());
-          }
-        }
-        diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) << II;
-        if (OriginalCallee != Callee) {
-          diag(OriginalCallee->getLocation(),
-               "actual callee (%0) is declared here", DiagnosticIDs::Note)
-              << OriginalCallee;
-        }
+      const bool HadMismatch = diagnoseMismatchInComment(
+          *this, Ctx, Callee->parameters(), II, StrictMode, IdentRE, Comment,
+          PVD->getLocation());
+      if (HadMismatch && OriginalCallee != Callee) {
+        diag(OriginalCallee->getLocation(),
+             "actual callee (%0) is declared here", DiagnosticIDs::Note)
+            << OriginalCallee;
       }
     }
 
     // If the argument comments are missing for literals add them.
     if (Comments.empty() && shouldAddComment(Args[I])) {
-      const std::string ArgComment =
-          (llvm::Twine("/*") + II->getName() + "=*/").str();
+      llvm::SmallString<32> ArgComment;
+      (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment);
       const DiagnosticBuilder Diag =
           diag(Args[I]->getBeginLoc(),
                "argument comment missing for literal argument %0")
@@ -346,6 +391,142 @@ void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
   }
 }
 
+void ArgumentCommentCheck::checkRecordInitializer(ASTContext *Ctx,
+                                                  const RecordDecl *RD,
+                                                  const InitListExpr *InitList,
+                                                  unsigned &InitIndex,
+                                                  SourceLocation &ArgBeginLoc) {
+  // If the record is not an aggregate (e.g. a base class with a user-declared
+  // constructor), we treat it as a single opaque element initialization.
+  bool IsAggregate = false;
+  if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD))
+    IsAggregate = CXXRD->isAggregate();
+  else
+    IsAggregate = true; // C struct
+
+  if (!IsAggregate) {
+    if (InitIndex < InitList->getNumInits()) {
+      const Expr *Arg = InitList->getInit(InitIndex);
+      ArgBeginLoc = Arg->getEndLoc();
+      InitIndex++;
+    }
+    return;
+  }
+
+  if (const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
+    for (const auto &Base : CXXRD->bases()) {
+      if (InitIndex >= InitList->getNumInits())
+        return;
+
+      const RecordDecl *BaseRD = Base.getType()->getAsRecordDecl();
+      if (!BaseRD) {
+        if (InitIndex < InitList->getNumInits()) {
+          const Expr *Arg = InitList->getInit(InitIndex);
+          ArgBeginLoc = Arg->getEndLoc();
+          InitIndex++;
+        }
+        continue;
+      }
+
+      const Expr *NextInit = InitList->getInit(InitIndex);
+      const QualType InitType = NextInit->getType();
+      const QualType BaseType = Base.getType();
+
+      // Check if this is an explicit initializer for the base.
+      const Expr *Stripped = NextInit->IgnoreParenImpCasts();
+      const auto *SubInitList = dyn_cast<InitListExpr>(Stripped);
+      const bool IsExplicitSubInit =
+          SubInitList &&
+          ASTContext::hasSameUnqualifiedType(SubInitList->getType(), BaseType);
+
+      if (IsExplicitSubInit) {
+        unsigned SubIndex = 0;
+        SourceLocation SubLoc = SubInitList->getLBraceLoc();
+        checkRecordInitializer(Ctx, BaseRD, SubInitList, SubIndex, SubLoc);
+        ArgBeginLoc = NextInit->getEndLoc();
+        InitIndex++;
+      } else if (ASTContext::hasSameUnqualifiedType(InitType, BaseType) ||
+                 (InitType->isRecordType() && BaseType->isRecordType() &&
+                  cast<CXXRecordDecl>(InitType->getAsRecordDecl())
+                      ->isDerivedFrom(cast<CXXRecordDecl>(BaseRD)))) {
+        // Copy/move construction or conversion from derived class.
+        // Treat as a single scalar initializer for the base.
+        ArgBeginLoc = NextInit->getEndLoc();
+        InitIndex++;
+      } else {
+        // Recursing into the current list.
+        checkRecordInitializer(Ctx, BaseRD, InitList, InitIndex, ArgBeginLoc);
+      }
+    }
+  }
+
+  for (const FieldDecl *FD : RD->fields()) {
+    if (InitIndex >= InitList->getNumInits())
+      break;
+
+    if (FD->isUnnamedBitField())
+      continue;
+
+    const Expr *Arg = InitList->getInit(InitIndex);
+
+    const IdentifierInfo *II = FD->getIdentifier();
+    if (!II) {
+      ArgBeginLoc = Arg->getEndLoc();
+      InitIndex++;
+      continue;
+    }
+
+    const llvm::SmallVector<std::pair<SourceLocation, StringRef>, 4> Comments =
+        collectLeadingComments(Ctx, ArgBeginLoc, Arg);
+    ArgBeginLoc = Arg->getEndLoc();
+
+    for (auto Comment : Comments)
+      (void)diagnoseMismatchInComment(*this, Ctx, RD->fields(), II, StrictMode,
+                                      IdentRE, Comment, FD->getLocation());
+
+    if (Comments.empty() && shouldAddComment(Arg)) {
+      llvm::SmallString<32> ArgComment;
+      (llvm::Twine("/*") + II->getName() + "=*/").toStringRef(ArgComment);
+      const DiagnosticBuilder Diag =
+          diag(Arg->getBeginLoc(),
+               "argument comment missing for literal argument %0")
+          << II << FixItHint::CreateInsertion(Arg->getBeginLoc(), ArgComment);
+    }
+
+    InitIndex++;
+  }
+}
+
+void ArgumentCommentCheck::checkInitList(ASTContext *Ctx,
+                                         const InitListExpr *InitList) {
+  const QualType Type = InitList->getType();
+  if (!Type->isRecordType())
+    return;
+  const RecordDecl *RD = Type->getAsRecordDecl();
+  if (!RD || RD->isUnion() ||
+      (isa<CXXRecordDecl>(RD) && !cast<CXXRecordDecl>(RD)->isAggregate()))
+    return;
+
+  if (const auto *InitListSyntactic = InitList->getSyntacticForm())
+    InitList = InitListSyntactic;
+
+  if (InitList->getNumInits() == 0 ||
+      (IgnoreSingleArgument && InitList->getNumInits() == 1))
+    return;
+
+  // If any designated initializers are present, we don't try to apply
+  // positional logic for argument comments, as the structure is no longer
+  // a simple positional match.
+  for (unsigned I = 0; I < InitList->getNumInits(); ++I) {
+    if (dyn_cast_or_null<DesignatedInitExpr>(InitList->getInit(I)))
+      return;
+  }
+
+  SourceLocation ArgBeginLoc = InitList->getLBraceLoc();
+  unsigned InitIndex = 0;
+  checkRecordInitializer(Ctx, RD, InitList, InitIndex, ArgBeginLoc);
+}
+
 void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
   if (const auto *Call = dyn_cast<CallExpr>(E)) {
@@ -355,8 +536,10 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
 
     checkCallArgs(Result.Context, Callee, Call->getCallee()->getEndLoc(),
                   llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
-  } else {
-    const auto *Construct = cast<CXXConstructExpr>(E);
+    return;
+  }
+
+  if (const auto *Construct = dyn_cast<CXXConstructExpr>(E)) {
     if (Construct->getNumArgs() > 0 &&
         Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) {
       // Ignore implicit construction.
@@ -366,6 +549,11 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
         Result.Context, Construct->getConstructor(),
         Construct->getParenOrBraceRange().getBegin(),
         llvm::ArrayRef(Construct->getArgs(), Construct->getNumArgs()));
+    return;
+  }
+
+  if (const auto *InitList = dyn_cast<InitListExpr>(E)) {
+    checkInitList(Result.Context, InitList);
   }
 }
 
diff --git a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
index 30fa32fad72e7..9e9df06b0d54b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.h
@@ -53,6 +53,11 @@ class ArgumentCommentCheck : public ClangTidyCheck {
                      SourceLocation ArgBeginLoc,
                      llvm::ArrayRef<const Expr *> Args);
 
+  void checkInitList(ASTContext *Ctx, const InitListExpr *InitList);
+  void checkRecordInitializer(ASTContext *Ctx, const RecordDecl *RD,
+                              const InitListExpr *InitList, unsigned &InitIndex,
+                              SourceLocation &ArgBeginLoc);
+
   bool shouldAddComment(const Expr *Arg) const;
 };
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d1fb1cba3e45a..3616c404fcfc0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -331,6 +331,11 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`bugprone-argument-comment
+  <clang-tidy/checks/bugprone/argument-comment>` check to support C++
+  aggregate initialization, including validation of comments against
+  struct field names and inheritance support.
+
 - Improved :doc:`bugprone-easily-swappable-parameters
   <clang-tidy/checks/bugprone/easily-swappable-parameters>` check by
   correcting a spelling mistake on its option
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst
index 8770d7224137a..29a1756be1c1e 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/argument-comment.rst
@@ -19,6 +19,21 @@ that are placed right before the argument.
 
 The check tries to detect typos and suggest automated fixes for them.
 
+The check also supports C++ aggregate initialization, allowing comments to be
+validated against struct and class field names, including those inherited from
+base classes.
+
+.. code-block:: c++
+
+  struct Base { int b; };
+  struct Derived : Base { int d; };
+
+  void f() {
+    Derived d1 = {/*b=*/1, /*d=*/2}; // OK
+    Derived d2 = {/*x=*/1, /*d=*/2};
+    // warning: argument name 'x' in comment does not match parameter name 'b'
+  }
+
 Options
 -------
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp
new file mode 100644
index 0000000000000..0ee6ad7e8196d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/argument-comment-struct-init.cpp
@@ -0,0 +1,132 @@
+// RUN: %check_clang_tidy -std=c++17 %s bugprone-argument-comment %t
+
+struct A {
+  int x, y;
+};
+
+void testA() {
+  A a1 = {/*x=*/1, /*y=*/2};
+  A a2 = {/*y=*/1, /*x=*/2};
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-2]]:20: warning: argument name 'x' in comment does not match parameter name 'y' [bugprone-argument-comment]
+
+  A a3 = {
+    /*x=*/1,
+    /*y=*/2
+  };
+
+  A a4 = {
+    /*y=*/1,
+    /*x=*/2
+  };
+  // CHECK-MESSAGES: [[@LINE-3]]:5: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
+  // CHECK-MESSAGES: [[@LINE-3]]:5: warning: argument name 'x' in comment does not match parameter name 'y' [bugprone-argument-comment]
+}
+
+struct B {
+  int x, y, z;
+};
+
+void testB() {
+  B b1 = {/*x=*/1, /*y=*/2}; // Partial init
+  B b2 = {/*z=*/1, /*y=*/2};
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: argument name 'z' in comment does not match parameter name 'x' [bugprone-argument-comment]
+}
+
+struct BitFields {
+  int a : 4;
+  int : 4;
+  int b : 4;
+};
+
+void testBitFields() {
+  BitFields b1 = {/*a=*/1, /*b=*/2};
+  BitFields b2 = {/*a=*/1, /*c=*/2};
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: argument name 'c' in comment does not match parameter name 'b' [bugprone-argument-comment]
+}
+
+struct CorrectFix {
+  int long_field_name;
+  int other;
+};
+
+void testFix() {
+  CorrectFix c = {/*long_feild_name=*/1, 2};
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: argument name 'long_feild_name' in comment does not match parameter name 'long_field_name' [bugprone-argument-comment]
+  // CHECK-FIXES: CorrectFix c = {/*long_field_name=*/1, 2};
+}
+
+struct Base {
+  int b;
+};
+
+struct Derived : Base {
+  in...
[truncated]

[vbvictor]

For me, it's hard to review per-commit in github UI.

Would you be willing to talk about what makes it difficult? (I've now made a couple of PRs with that review style in mind, so I'm wondering whether I could do something differently, like the stacking you mention, to make it easier).

Lets say we have commits A, B, C, D.
IF I do per-commit review, after the first round of review I need to mentally remember for later that e.g. A, C are LGTM and B, D are "bad". With Pr for each commit, I can explicitly accept/decline and forget about it.
Also, there could be problems when e.g. commit A changed one part that I don't like (so I comment) but commit B then changes this same part to a thing I like (so I have to undo the previous comment because now it looks good).

P.S. Your PRS I reviewed in one go, luckily they weren't too big.