https://github.com/nvetrini created https://github.com/llvm/llvm-project/pull/199411
The location of commas inside a ParenListExpr were not saved when parsing, leading to an incorrect location of the indicated comma when using BinaryOperator::getOperatorLoc() on the comma operator. To avoid this, an additional parameter is added to ParenListExpr to store the location of commas. >From de32ef90ebe5e2145a57ed111615b1829e7146bb Mon Sep 17 00:00:00 2001 From: Nicola Vetrini <[email protected]> Date: Sun, 24 May 2026 13:06:55 +0200 Subject: [PATCH] [clang] Fix comma location tracking inside ParenListExpr. The location of commas inside a ParenListExpr were not saved when parsing, leading to an incorrect location of the indicated comma when using BinaryOperator::getOperatorLoc() on the comma operator. To avoid this, an additional parameter is added to ParenListExpr to store the location of commas. A test is added to prevent regressions. Co-Authored-By: gpt-5.5 --- clang/include/clang/AST/Expr.h | 41 ++++++++++++++----- clang/include/clang/Parse/Parser.h | 4 +- clang/include/clang/Sema/Sema.h | 3 +- clang/lib/AST/Expr.cpp | 35 ++++++++++------ clang/lib/Parse/ParseExpr.cpp | 14 +++++-- clang/lib/Sema/SemaExpr.cpp | 18 ++++---- clang/lib/Serialization/ASTReaderStmt.cpp | 24 ++++++++--- clang/lib/Serialization/ASTWriterStmt.cpp | 3 ++ .../warn-comma-operator-paren-list-cast.c | 7 ++++ 9 files changed, 108 insertions(+), 41 deletions(-) create mode 100644 clang/test/Sema/warn-comma-operator-paren-list-cast.c diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index b91bf4a5375fb..b53b77383296d 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -6083,31 +6083,44 @@ class ImplicitValueInitExpr : public Expr { class ParenListExpr final : public Expr, - private llvm::TrailingObjects<ParenListExpr, Stmt *> { + private llvm::TrailingObjects<ParenListExpr, Stmt *, SourceLocation> { friend class ASTStmtReader; friend TrailingObjects; /// The location of the left and right parentheses. SourceLocation LParenLoc, RParenLoc; + /// The number of comma locations stored after the expression list. + unsigned NumCommas; + + size_t numTrailingObjects(OverloadToken<Stmt *>) const { + return getNumExprs(); + } + + size_t numTrailingObjects(OverloadToken<SourceLocation>) const { + return NumCommas; + } + /// Build a paren list. ParenListExpr(SourceLocation LParenLoc, ArrayRef<Expr *> Exprs, - SourceLocation RParenLoc); + SourceLocation RParenLoc, ArrayRef<SourceLocation> CommaLocs); /// Build an empty paren list. - ParenListExpr(EmptyShell Empty, unsigned NumExprs); + ParenListExpr(EmptyShell Empty, unsigned NumExprs, unsigned NumCommas); public: /// Create a paren list. static ParenListExpr *Create(const ASTContext &Ctx, SourceLocation LParenLoc, - ArrayRef<Expr *> Exprs, - SourceLocation RParenLoc); + ArrayRef<Expr *> Exprs, SourceLocation RParenLoc, + ArrayRef<SourceLocation> CommaLocs = {}); /// Create an empty paren list. - static ParenListExpr *CreateEmpty(const ASTContext &Ctx, unsigned NumExprs); + static ParenListExpr *CreateEmpty(const ASTContext &Ctx, unsigned NumExprs, + unsigned NumCommas = 0); /// Return the number of expressions in this paren list. unsigned getNumExprs() const { return ParenListExprBits.NumExprs; } + unsigned getNumCommas() const { return NumCommas; } Expr *getExpr(unsigned Init) { assert(Init < getNumExprs() && "Initializer access out of range!"); @@ -6118,14 +6131,20 @@ class ParenListExpr final return const_cast<ParenListExpr *>(this)->getExpr(Init); } - Expr **getExprs() { return reinterpret_cast<Expr **>(getTrailingObjects()); } + Expr **getExprs() { + return reinterpret_cast<Expr **>(getTrailingObjects<Stmt *>()); + } Expr *const *getExprs() const { - return reinterpret_cast<Expr *const *>(getTrailingObjects()); + return reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>()); } ArrayRef<Expr *> exprs() const { return {getExprs(), getNumExprs()}; } + ArrayRef<SourceLocation> getCommaLocs() const { + return {getTrailingObjects<SourceLocation>(), getNumCommas()}; + } + SourceLocation getLParenLoc() const { return LParenLoc; } SourceLocation getRParenLoc() const { return RParenLoc; } SourceLocation getBeginLoc() const { return getLParenLoc(); } @@ -6137,10 +6156,12 @@ class ParenListExpr final // Iterators child_range children() { - return child_range(getTrailingObjects(getNumExprs())); + return child_range(getTrailingObjects<Stmt *>(), + getTrailingObjects<Stmt *>() + getNumExprs()); } const_child_range children() const { - return const_child_range(getTrailingObjects(getNumExprs())); + return const_child_range(getTrailingObjects<Stmt *>(), + getTrailingObjects<Stmt *>() + getNumExprs()); } }; diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h index c6c492b4980af..0d8570161eeec 100644 --- a/clang/include/clang/Parse/Parser.h +++ b/clang/include/clang/Parse/Parser.h @@ -4250,7 +4250,9 @@ class Parser : public CodeCompletionHandler { /// assignment-expression /// simple-expression-list , assignment-expression /// \endverbatim - bool ParseSimpleExpressionList(SmallVectorImpl<Expr *> &Exprs); + bool ParseSimpleExpressionList( + SmallVectorImpl<Expr *> &Exprs, + SmallVectorImpl<SourceLocation> *CommaLocs = nullptr); /// This parses the unit that starts with a '(' token, based on what is /// allowed by ExprType. The actual thing parsed is returned in ExprType. If diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index e71794b2d92c9..b2c6f16479375 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -7361,7 +7361,8 @@ class Sema final : public SemaBase { Scope *UDLScope = nullptr); ExprResult ActOnParenExpr(SourceLocation L, SourceLocation R, Expr *E); ExprResult ActOnParenListExpr(SourceLocation L, SourceLocation R, - MultiExprArg Val); + MultiExprArg Val, + ArrayRef<SourceLocation> CommaLocs = {}); ExprResult ActOnCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T, unsigned NumUserSpecifiedExprs, SourceLocation InitLoc, diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 90747be4208e1..cb66f89b9a7ed 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -4958,33 +4958,42 @@ SourceLocation DesignatedInitUpdateExpr::getEndLoc() const { } ParenListExpr::ParenListExpr(SourceLocation LParenLoc, ArrayRef<Expr *> Exprs, - SourceLocation RParenLoc) + SourceLocation RParenLoc, + ArrayRef<SourceLocation> CommaLocs) : Expr(ParenListExprClass, QualType(), VK_PRValue, OK_Ordinary), - LParenLoc(LParenLoc), RParenLoc(RParenLoc) { + LParenLoc(LParenLoc), RParenLoc(RParenLoc), NumCommas(CommaLocs.size()) { + assert((CommaLocs.empty() || CommaLocs.size() + 1 == Exprs.size()) && + "wrong number of comma locations for paren list"); ParenListExprBits.NumExprs = Exprs.size(); - llvm::copy(Exprs, getTrailingObjects()); + llvm::copy(Exprs, getTrailingObjects<Stmt *>()); + llvm::copy(CommaLocs, getTrailingObjects<SourceLocation>()); setDependence(computeDependence(this)); } -ParenListExpr::ParenListExpr(EmptyShell Empty, unsigned NumExprs) - : Expr(ParenListExprClass, Empty) { +ParenListExpr::ParenListExpr(EmptyShell Empty, unsigned NumExprs, + unsigned NumCommas) + : Expr(ParenListExprClass, Empty), NumCommas(NumCommas) { ParenListExprBits.NumExprs = NumExprs; } ParenListExpr *ParenListExpr::Create(const ASTContext &Ctx, SourceLocation LParenLoc, ArrayRef<Expr *> Exprs, - SourceLocation RParenLoc) { - void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(Exprs.size()), - alignof(ParenListExpr)); - return new (Mem) ParenListExpr(LParenLoc, Exprs, RParenLoc); + SourceLocation RParenLoc, + ArrayRef<SourceLocation> CommaLocs) { + void *Mem = Ctx.Allocate( + totalSizeToAlloc<Stmt *, SourceLocation>(Exprs.size(), CommaLocs.size()), + alignof(ParenListExpr)); + return new (Mem) ParenListExpr(LParenLoc, Exprs, RParenLoc, CommaLocs); } ParenListExpr *ParenListExpr::CreateEmpty(const ASTContext &Ctx, - unsigned NumExprs) { - void *Mem = - Ctx.Allocate(totalSizeToAlloc<Stmt *>(NumExprs), alignof(ParenListExpr)); - return new (Mem) ParenListExpr(EmptyShell(), NumExprs); + unsigned NumExprs, + unsigned NumCommas) { + void *Mem = Ctx.Allocate( + totalSizeToAlloc<Stmt *, SourceLocation>(NumExprs, NumCommas), + alignof(ParenListExpr)); + return new (Mem) ParenListExpr(EmptyShell(), NumExprs, NumCommas); } /// Certain overflow-dependent code patterns can have their integer overflow diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp index e38481f05da63..c0bc7c731fb31 100644 --- a/clang/lib/Parse/ParseExpr.cpp +++ b/clang/lib/Parse/ParseExpr.cpp @@ -2941,8 +2941,9 @@ Parser::ParseParenExpression(ParenParseOption &ExprType, bool StopIfCastExpr, // Parse the expression-list. InMessageExpressionRAIIObject InMessage(*this, false); ExprVector ArgExprs; + SmallVector<SourceLocation, 4> CommaLocs; - if (!ParseSimpleExpressionList(ArgExprs)) { + if (!ParseSimpleExpressionList(ArgExprs, &CommaLocs)) { // FIXME: If we ever support comma expressions as operands to // fold-expressions, we'll need to allow multiple ArgExprs here. if (ExprType >= ParenParseOption::FoldExpr && ArgExprs.size() == 1 && @@ -2952,8 +2953,8 @@ Parser::ParseParenExpression(ParenParseOption &ExprType, bool StopIfCastExpr, } ExprType = ParenParseOption::SimpleExpr; - Result = Actions.ActOnParenListExpr(OpenLoc, Tok.getLocation(), - ArgExprs); + Result = Actions.ActOnParenListExpr(OpenLoc, Tok.getLocation(), ArgExprs, + CommaLocs); } } else if (getLangOpts().OpenMP >= 50 && OpenMPDirectiveParsing && ExprType == ParenParseOption::CastExpr && Tok.is(tok::l_square) && @@ -3277,7 +3278,9 @@ bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs, return SawError; } -bool Parser::ParseSimpleExpressionList(SmallVectorImpl<Expr *> &Exprs) { +bool Parser::ParseSimpleExpressionList( + SmallVectorImpl<Expr *> &Exprs, + SmallVectorImpl<SourceLocation> *CommaLocs) { while (true) { ExprResult Expr = ParseAssignmentExpression(); if (Expr.isInvalid()) @@ -3292,6 +3295,9 @@ bool Parser::ParseSimpleExpressionList(SmallVectorImpl<Expr *> &Exprs) { // Move to the next argument, remember where the comma was. Token Comma = Tok; + if (CommaLocs) + CommaLocs->push_back(Comma.getLocation()); + ConsumeToken(); checkPotentialAngleBracketDelimiter(Comma); } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 521a8516ac179..d5f8172c03a8a 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -8318,20 +8318,24 @@ Sema::MaybeConvertParenListExprToParenExpr(Scope *S, Expr *OrigExpr) { return OrigExpr; ExprResult Result(E->getExpr(0)); + ArrayRef<SourceLocation> CommaLocs = E->getCommaLocs(); - for (unsigned i = 1, e = E->getNumExprs(); i != e && !Result.isInvalid(); ++i) - Result = ActOnBinOp(S, E->getExprLoc(), tok::comma, Result.get(), - E->getExpr(i)); + for (unsigned i = 1, e = E->getNumExprs(); i != e && !Result.isInvalid(); + ++i) { + SourceLocation CommaLoc = + i - 1 < CommaLocs.size() ? CommaLocs[i - 1] : E->getLParenLoc(); + Result = ActOnBinOp(S, CommaLoc, tok::comma, Result.get(), E->getExpr(i)); + } if (Result.isInvalid()) return ExprError(); return ActOnParenExpr(E->getLParenLoc(), E->getRParenLoc(), Result.get()); } -ExprResult Sema::ActOnParenListExpr(SourceLocation L, - SourceLocation R, - MultiExprArg Val) { - return ParenListExpr::Create(Context, L, Val, R); +ExprResult Sema::ActOnParenListExpr(SourceLocation L, SourceLocation R, + MultiExprArg Val, + ArrayRef<SourceLocation> CommaLocs) { + return ParenListExpr::Create(Context, L, Val, R, CommaLocs); } ExprResult Sema::ActOnCXXParenListInitExpr(ArrayRef<Expr *> Args, QualType T, diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp index 7e51ce8c0aca2..2b298d3cbca3c 100644 --- a/clang/lib/Serialization/ASTReaderStmt.cpp +++ b/clang/lib/Serialization/ASTReaderStmt.cpp @@ -748,9 +748,17 @@ void ASTStmtReader::VisitParenListExpr(ParenListExpr *E) { unsigned NumExprs = Record.readInt(); assert((NumExprs == E->getNumExprs()) && "Wrong NumExprs!"); for (unsigned I = 0; I != NumExprs; ++I) - E->getTrailingObjects()[I] = Record.readSubStmt(); + E->getTrailingObjects<Stmt *>()[I] = Record.readSubStmt(); E->LParenLoc = readSourceLocation(); E->RParenLoc = readSourceLocation(); + if (Record.getIdx() < Record.size()) { + unsigned NumCommas = Record.readInt(); + assert((NumCommas == E->getNumCommas()) && "Wrong NumCommas!"); + for (unsigned I = 0; I != NumCommas; ++I) + E->getTrailingObjects<SourceLocation>()[I] = readSourceLocation(); + } else { + assert(E->getNumCommas() == 0 && "missing comma locations"); + } } void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) { @@ -3303,11 +3311,17 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) { S = new (Context) ParenExpr(Empty); break; - case EXPR_PAREN_LIST: - S = ParenListExpr::CreateEmpty( - Context, - /* NumExprs=*/Record[ASTStmtReader::NumExprFields]); + case EXPR_PAREN_LIST: { + unsigned NumExprs = Record[ASTStmtReader::NumExprFields]; + unsigned NumCommas = 0; + unsigned CommaCountIdx = ASTStmtReader::NumExprFields + 1 + NumExprs + 2; + if (Record.size() > CommaCountIdx) + NumCommas = Record[CommaCountIdx]; + S = ParenListExpr::CreateEmpty(Context, + /* NumExprs=*/NumExprs, + /* NumCommas=*/NumCommas); break; + } case EXPR_UNARY_OPERATOR: { BitsUnpacker UnaryOperatorBits(Record[ASTStmtReader::NumStmtFields]); diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp index a7e815a1ef438..1009c494ef0c1 100644 --- a/clang/lib/Serialization/ASTWriterStmt.cpp +++ b/clang/lib/Serialization/ASTWriterStmt.cpp @@ -847,6 +847,9 @@ void ASTStmtWriter::VisitParenListExpr(ParenListExpr *E) { Record.AddStmt(SubStmt); Record.AddSourceLocation(E->getLParenLoc()); Record.AddSourceLocation(E->getRParenLoc()); + Record.push_back(E->getNumCommas()); + for (SourceLocation CommaLoc : E->getCommaLocs()) + Record.AddSourceLocation(CommaLoc); Code = serialization::EXPR_PAREN_LIST; } diff --git a/clang/test/Sema/warn-comma-operator-paren-list-cast.c b/clang/test/Sema/warn-comma-operator-paren-list-cast.c new file mode 100644 index 0000000000000..b15a065c72a91 --- /dev/null +++ b/clang/test/Sema/warn-comma-operator-paren-list-cast.c @@ -0,0 +1,7 @@ +// RUN: %clang_cc1 -fsyntax-only -Wcomma -fno-caret-diagnostics %s 2>&1 | FileCheck %s + +void comma_in_paren_list_cast(void) { + int x; + (void)(int)(x = 0, 1); + // CHECK: :[[@LINE-1]]:20: warning: possible misuse of comma operator here +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
