[Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

[riccibruno]

Summary

NFCs factored out of the following patches:

• Change all of the Expr * to const Expr * in SequenceChecker for const-correctness
• Add some comments
• clang-format

[aaron.ballman]

lib/Sema/SemaChecking.cpp

11748 ↗

(On Diff #184966)

You can drop the {} here and below.

11927 ↗

(On Diff #184966)

A last thing we have to be careful is that -> We also have to be careful that

[riccibruno]

lib/Sema/SemaChecking.cpp

11748 ↗

(On Diff #184966)

I don't think I can (although I can drop it for the array of SequenceTree::Seq). I want to value-initialize each expression in this array so that they are all null.

[riccibruno]

Rebased and fixed a comment.

[riccibruno]

Rebased.

[xbolva00]

This seems ok (not sure why stalled)

[riccibruno]

Rebased. Thanks for the review @xbolva00 !

[riccibruno]

Rebased on top of current master. No need to look at it.

clang/lib/Sema/SemaChecking.cpp

enum UsageKind {
 
     /// A modification of an object which is not sequenced before the value
     /// computation of the expression, such as n++.
     UK_ModAsSideEffect,
 
     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<Object, UsageInfo, 16>;
 
   Sema &SemaRef;
 
   /// Sequenced regions within the expression.
   SequenceTree Tree;
 
class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
   /// UK_ModAsValue.
   struct SequencedSubexpression {
     SequencedSubexpression(SequenceChecker &Self)
       : Self(Self), OldModAsSideEffect(Self.ModAsSideEffect) {
       Self.ModAsSideEffect = &ModAsSideEffect;
     }
 
     ~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<Object, Usage> &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;
     }
 
     SequenceChecker &Self;
     SmallVector<std::pair<Object, Usage>, 4> ModAsSideEffect;
     SmallVectorImpl<std::pair<Object, Usage>> *OldModAsSideEffect;
   };
if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
         return ME->getMemberDecl();
     } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
       // FIXME: If this is a reference, map through to its value.
       return DRE->getDecl();
     return nullptr;
   }
 
   /// 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);
 
     SemaRef.DiagRuntimeBehavior(
         Mod->getExprLoc(), {Mod, ModOrUse},
         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:
   SequenceChecker(Sema &S, Expr *E, SmallVectorImpl<Expr *> &WorkList)
       : Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) {
     Visit(E);
   }