Index: clang/lib/Sema/SemaChecking.cpp =================================================================== --- clang/lib/Sema/SemaChecking.cpp +++ clang/lib/Sema/SemaChecking.cpp @@ -12161,20 +12161,38 @@ UK_Count = UK_ModAsSideEffect + 1 }; + /// Bundle together a sequencing region and the expression corresponding + /// to a specific usage. One Usage is stored for each usage kind in UsageInfo. struct Usage { - Expr *Use; + Expr *UsageExpr = nullptr; SequenceTree::Seq Seq; - - Usage() : Use(nullptr), Seq() {} + Usage() = default; + Usage(Expr *UsageExpr, SequenceTree::Seq Seq) + : UsageExpr(UsageExpr), Seq(Seq) {} }; - struct UsageInfo { - Usage Uses[UK_Count]; + class UsageInfo { + /// The expressions corresponding to each usage kind. + Expr *UsageExprs[UK_Count]{}; + + /// The sequencing regions corresponding to each usage kind. + SequenceTree::Seq Seqs[UK_Count]{}; - /// Have we issued a diagnostic for this variable already? - bool Diagnosed; + /// Have we issued a diagnostic for this object already? + bool Diagnosed = false; - UsageInfo() : Uses(), Diagnosed(false) {} + public: + Usage getUsage(UsageKind UK) const { + assert(UK < UK_Count && "Invalid UsageKind!"); + return Usage{UsageExprs[UK], Seqs[UK]}; + } + void setUsage(UsageKind UK, Usage U) { + assert(UK < UK_Count && "Invalid UsageKind!"); + UsageExprs[UK] = U.UsageExpr; + Seqs[UK] = U.Seq; + } + void markDiagnosed() { Diagnosed = true; } + bool alreadyDiagnosed() const { return Diagnosed; } }; using UsageInfoMap = llvm::SmallDenseMap; @@ -12209,11 +12227,14 @@ } ~SequencedSubexpression() { - for (auto &M : llvm::reverse(ModAsSideEffect)) { - UsageInfo &U = Self.UsageMap[M.first]; - auto &SideEffectUsage = U.Uses[UK_ModAsSideEffect]; - Self.addUsage(U, M.first, SideEffectUsage.Use, UK_ModAsValue); - SideEffectUsage = M.second; + for (std::pair &M : llvm::reverse(ModAsSideEffect)) { + // Add a new usage with usage kind UK_ModAsValue, and then restore + // the previous usage with UK_ModAsSideEffect (thus clearing it if + // the previous one was empty). + UsageInfo &UI = Self.UsageMap[M.first]; + Usage SideEffectUsage = UI.getUsage(UK_ModAsSideEffect); + Self.addUsage(M.first, UI, SideEffectUsage.UsageExpr, UK_ModAsValue); + UI.setUsage(UK_ModAsSideEffect, M.second); } Self.ModAsSideEffect = OldModAsSideEffect; } @@ -12277,28 +12298,34 @@ } /// Note that an object was modified or used by an expression. - void addUsage(UsageInfo &UI, Object O, Expr *Ref, UsageKind UK) { - Usage &U = UI.Uses[UK]; - if (!U.Use || !Tree.isUnsequenced(Region, U.Seq)) { + /// UI is the UsageInfo for the object O as obtained via the UsageMap. + void addUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind UK) { + // Get the old usage for the given object and usage kind. + Usage U = UI.getUsage(UK); + if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) { + // If we have a modification as side effect and are in a sequenced + // subexpression, save the old Usage so that we can restore it later + // in SequencedSubexpression::~SequencedSubexpression. if (UK == UK_ModAsSideEffect && ModAsSideEffect) ModAsSideEffect->push_back(std::make_pair(O, U)); - U.Use = Ref; - U.Seq = Region; + // Then record the new usage with the current sequencing region. + UI.setUsage(UK, Usage(UsageExpr, Region)); } } /// Check whether a modification or use conflicts with a prior usage. - void checkUsage(Object O, UsageInfo &UI, Expr *Ref, UsageKind OtherKind, + /// UI is the UsageInfo for the object O as obtained via the UsageMap. + void checkUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind OtherKind, bool IsModMod) { - if (UI.Diagnosed) + if (UI.alreadyDiagnosed()) return; - const Usage &U = UI.Uses[OtherKind]; - if (!U.Use || !Tree.isUnsequenced(Region, U.Seq)) + Usage U = UI.getUsage(OtherKind); + if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) return; - Expr *Mod = U.Use; - Expr *ModOrUse = Ref; + Expr *Mod = U.UsageExpr; + Expr *ModOrUse = UsageExpr; if (OtherKind == UK_Use) std::swap(Mod, ModOrUse); @@ -12307,32 +12334,60 @@ SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod : diag::warn_unsequenced_mod_use) << O << SourceRange(ModOrUse->getExprLoc())); - UI.Diagnosed = true; + UI.markDiagnosed(); } - void notePreUse(Object O, Expr *Use) { - UsageInfo &U = UsageMap[O]; + // A note on note{Pre, Post}{Use, Mod}: + // + // (It helps to follow the algorithm with an expression such as + // "((++k)++, k) = k" or "k = (k++, k++)". Both contain unsequenced + // operations before C++17 and both are well-defined in C++17). + // + // When visiting a node which uses/modify an object we first call notePreUse + // or notePreMod before visiting its sub-expression(s). At this point the + // children of the current node have not yet been visited and so the eventual + // uses/modifications resulting from the children of the current node have not + // been recorded yet. + // + // We then visit the children of the current node. After that notePostUse or + // notePostMod is called. These will 1) detect an unsequenced modification + // as side effect (as in "k++ + k") and 2) add a new usage with the + // appropriate usage kind. + // + // We also have to be careful that some operation sequences modification as + // side effect as well (for example: || or ,). To account for this we wrap + // the visitation of such a sub-expression (for example: the LHS of || or ,) + // with SequencedSubexpression. SequencedSubexpression is an RAII object + // which record usages which are modifications as side effect, and then + // downgrade them (or more accurately restore the previous usage which was a + // modification as side effect) when exiting the scope of the sequenced + // subexpression. + + void notePreUse(Object O, Expr *UseExpr) { + UsageInfo &UI = UsageMap[O]; // Uses conflict with other modifications. - checkUsage(O, U, Use, UK_ModAsValue, false); + checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/false); } - void notePostUse(Object O, Expr *Use) { - UsageInfo &U = UsageMap[O]; - checkUsage(O, U, Use, UK_ModAsSideEffect, false); - addUsage(U, O, Use, UK_Use); + void notePostUse(Object O, Expr *UseExpr) { + UsageInfo &UI = UsageMap[O]; + checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect, + /*IsModMod=*/false); + addUsage(O, UI, UseExpr, /*UsageKind=*/UK_Use); } - void notePreMod(Object O, Expr *Mod) { - UsageInfo &U = UsageMap[O]; + void notePreMod(Object O, Expr *ModExpr) { + UsageInfo &UI = UsageMap[O]; // Modifications conflict with other modifications and with uses. - checkUsage(O, U, Mod, UK_ModAsValue, true); - checkUsage(O, U, Mod, UK_Use, false); + checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/true); + checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_Use, /*IsModMod=*/false); } - void notePostMod(Object O, Expr *Use, UsageKind UK) { - UsageInfo &U = UsageMap[O]; - checkUsage(O, U, Use, UK_ModAsSideEffect, true); - addUsage(U, O, Use, UK); + void notePostMod(Object O, Expr *ModExpr, UsageKind UK) { + UsageInfo &UI = UsageMap[O]; + checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect, + /*IsModMod=*/true); + addUsage(O, UI, ModExpr, /*UsageKind=*/UK); } public: