Thread Safety Analysis: some minor cleanups to the latest thread safety changes. No functional changes intended.

* Adds an iterator_range interface to CallExpr to get the arguments
* Modifies SExpr such that it must be allocated in the Arena, and cannot be deleted
* Minor const-correctness and nullptr updates
* Adds some operator!= implementations to complement operator==
* Removes unused functionality

llvm-svn: 205915

clang/include/clang/AST/Expr.h

@@ -2227,6 +2227,13 @@ public:
 
   typedef ExprIterator arg_iterator;
   typedef ConstExprIterator const_arg_iterator;
+  typedef llvm::iterator_range<arg_iterator> arg_range;
+  typedef llvm::iterator_range<const_arg_iterator> arg_const_range;
+
+  arg_range arguments() { return arg_range(arg_begin(), arg_end()); }
+  arg_const_range arguments() const {
+    return arg_const_range(arg_begin(), arg_end());
+  }
 
   arg_iterator arg_begin() { return SubExprs+PREARGS_START+getNumPreArgs(); }
   arg_iterator arg_end() {

clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h

@@ -42,35 +42,32 @@
 namespace clang {
 namespace threadSafety {
 
-
+// CFG traversal uses templates instead of virtual function dispatch. Visitors
-// Simple Visitor class for traversing a clang CFG.
+// must implement the following functions:
-class CFGVisitor {
+//
-public:
+// Enter the CFG for Decl D, and perform any initial setup operations.
-  // Enter the CFG for Decl D, and perform any initial setup operations.
+// void enterCFG(CFG *Cfg, const NamedDecl *D, const CFGBlock *First) {}
-  void enterCFG(CFG *Cfg, const NamedDecl *D, const CFGBlock *First) {}
+//
-
+// Enter a CFGBlock.
-  // Enter a CFGBlock.
+// void enterCFGBlock(const CFGBlock *B) {}
-  void enterCFGBlock(const CFGBlock *B) {}
+//
-
+// Process an ordinary statement.
-  // Process an ordinary statement.
+// void handleStatement(const Stmt *S) {}
-  void handleStatement(const Stmt *S) {}
+//
-
+// Process a destructor call
-  // Process a destructor call
+// void handleDestructorCall(const VarDecl *VD, const CXXDestructorDecl *DD) {}
-  void handleDestructorCall(const VarDecl *VD, const CXXDestructorDecl *DD) {}
+//
-
+// Process a successor edge.
-  // Process a successor edge.
+// void handleSuccessor(const CFGBlock *Succ) {}
-  void handleSuccessor(const CFGBlock *Succ) {}
+//
-
+// Process a successor back edge to a previously visited block.
-  // Process a successor back edge to a previously visited block.
+// void handleSuccessorBackEdge(const CFGBlock *Succ) {}
-  void handleSuccessorBackEdge(const CFGBlock *Succ) {}
+//
-
+// Leave a CFGBlock.
-  // Leave a CFGBlock.
+// void exitCFGBlock(const CFGBlock *B) {}
-  void exitCFGBlock(const CFGBlock *B) {}
+//
-
+// Leave the CFG, and perform any final cleanup operations.
-  // Leave the CFG, and perform any final cleanup operations.
+// void exitCFG(const CFGBlock *Last) {}
-  void exitCFG(const CFGBlock *Last) {}
-};
-
 
 // Walks the clang CFG, and invokes methods on a given CFGVisitor.
 class CFGWalker {
@@ -104,13 +101,13 @@ public:
 
     V.enterCFG(CFGraph, FDecl, &CFGraph->getEntry());
 
-    for (const CFGBlock* CurrBlock : *SortedGraph) {
+    for (const auto *CurrBlock : *SortedGraph) {
       VisitedBlocks.insert(CurrBlock);
 
       V.enterCFGBlock(CurrBlock);
 
       // Process statements
-      for (auto BI : *CurrBlock) {
+      for (const auto &BI : *CurrBlock) {
         switch (BI.getKind()) {
         case CFGElement::Statement: {
           V.handleStatement(BI.castAs<CFGStmt>().getStmt());

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

@@ -207,6 +207,10 @@ public:
   //   compare all subexpressions, following the comparator interface
   // }
 
+  void *operator new(size_t S, clang::threadSafety::til::MemRegionRef &R) {
+    return ::operator new(S, R);
+  }
+
 protected:
   SExpr(TIL_Opcode Op) : Opcode(Op), Reserved(0), Flags(0) {}
   SExpr(const SExpr &E) : Opcode(E.Opcode), Reserved(0), Flags(E.Flags) {}
@@ -216,7 +220,15 @@ protected:
   unsigned short Flags;
 
 private:
-  SExpr();
+  // Note, this cannot be explicitly deleted due to initializers automatically
+  // referencing destructor declarations. However, it does not need to be
+  // defined because that reference does not require an definition.
+  ~SExpr();
+  SExpr() = delete;
+
+  // SExpr objects must be created in an arena and cannot be deleted.
+  void *operator new(size_t) = delete;
+  void operator delete(void *) = delete;
 };
 
 
@@ -227,7 +239,7 @@ class SExprRef {
 public:
   SExprRef() : Ptr(nullptr) { }
   SExprRef(std::nullptr_t P) : Ptr(nullptr) { }
-  SExprRef(SExprRef &&R) : Ptr(R.Ptr) { }
+  SExprRef(SExprRef &&R) : Ptr(R.Ptr) { R.Ptr = nullptr; }
 
   // Defined after Variable and Future, below.
   inline SExprRef(SExpr *P);
@@ -242,9 +254,12 @@ public:
   SExpr       &operator*()        { return *Ptr; }
   const SExpr &operator*() const  { return *Ptr; }
 
-  bool operator==(const SExprRef& R) const { return Ptr == R.Ptr; }
+  bool operator==(const SExprRef &R) const { return Ptr == R.Ptr; }
-  bool operator==(const SExpr* P)    const { return Ptr == P; }
+  bool operator!=(const SExprRef &R) const { return !operator==(R); }
-  bool operator==(std::nullptr_t P)  const { return Ptr == nullptr; }
+  bool operator==(const SExpr *P) const { return Ptr == P; }
+  bool operator!=(const SExpr *P) const { return !operator==(P); }
+  bool operator==(std::nullptr_t) const { return Ptr == nullptr; }
+  bool operator!=(std::nullptr_t) const { return Ptr != nullptr; }
 
   inline void reset(SExpr *E);
 
@@ -252,28 +267,17 @@ private:
   inline void attach();
   inline void detach();
 
-  SExprRef(const SExprRef& R) : Ptr(R.Ptr) { }
-
   SExpr *Ptr;
 };
 
 
 // Contains various helper functions for SExprs.
-class ThreadSafetyTIL {
+namespace ThreadSafetyTIL {
-public:
+static bool isTrivial(SExpr *E) {
-  static const int MaxOpcode = COP_MAX;
+  unsigned Op = E->opcode();
-
+  return Op == COP_Variable || Op == COP_Literal || Op == COP_LiteralPtr;
-  static inline bool isTrivial(SExpr *E) {
+}
-    unsigned Op = E->opcode();
+}
-    return Op == COP_Variable || Op == COP_Literal || Op == COP_LiteralPtr;
-  }
-
-  static inline bool isLargeValue(SExpr *E) {
-    unsigned Op = E->opcode();
-    return (Op >= COP_Function && Op <= COP_Code);
-  }
-};
-
 
 class Function;
 class SFunction;
@@ -311,7 +315,7 @@ public:
 
   VariableKind kind() const { return static_cast<VariableKind>(Flags); }
 
-  StringRef name() const { return Cvdecl ? Cvdecl->getName() : "_x"; }
+  const StringRef name() const { return Cvdecl ? Cvdecl->getName() : "_x"; }
   const clang::ValueDecl *clangDecl() const { return Cvdecl; }
 
   // Returns the definition (for let vars) or type (for parameter & self vars)
@@ -320,8 +324,8 @@ public:
   void attachVar() const { ++NumUses; }
   void detachVar() const { assert(NumUses > 0); --NumUses; }
 
-  unsigned getID() { return Id; }
+  unsigned getID() const { return Id; }
-  unsigned getBlockID() { return BlockID; }
+  unsigned getBlockID() const { return BlockID; }
 
   void setID(unsigned Bid, unsigned I) {
     BlockID = static_cast<unsigned short>(Bid);
@@ -369,7 +373,7 @@ public:
   Future() :
     SExpr(COP_Future), Status(FS_pending), Result(nullptr), Location(nullptr)
   {}
-  virtual ~Future() {}
+  virtual ~Future() = delete;
 
   // Registers the location in the AST where this future is stored.
   // Forcing the future will automatically update the AST.

clang/lib/Analysis/ThreadSafetyCommon.cpp

@@ -37,11 +37,11 @@ typedef SExprBuilder::CallingContext CallingContext;
 
 til::SExpr *SExprBuilder::lookupStmt(const Stmt *S) {
   if (!SMap)
-    return 0;
+    return nullptr;
   auto It = SMap->find(S);
   if (It != SMap->end())
     return It->second;
-  return 0;
+  return nullptr;
 }
 
 void SExprBuilder::insertStmt(const Stmt *S, til::Variable *V) {
@@ -160,8 +160,8 @@ til::SExpr *SExprBuilder::translateCallExpr(const CallExpr *CE,
                                             CallingContext *Ctx) {
   // TODO -- Lock returned
   til::SExpr *E = translate(CE->getCallee(), Ctx);
-  for (unsigned I = 0, N = CE->getNumArgs(); I < N; ++I) {
+  for (const auto *Arg : CE->arguments()) {
-    til::SExpr *A = translate(CE->getArg(I), Ctx);
+    til::SExpr *A = translate(Arg, Ctx);
     E = new (Arena) til::Apply(E, A);
   }
   return new (Arena) til::Call(E, CE);
@@ -200,10 +200,9 @@ til::SExpr *SExprBuilder::translateUnaryOperator(const UnaryOperator *UO,
   case UO_LNot:
   case UO_Real:
   case UO_Imag:
-  case UO_Extension: {
+  case UO_Extension:
-    til::SExpr *E0 = translate(UO->getSubExpr(), Ctx);
+    return new (Arena)
-    return new (Arena) til::UnaryOp(UO->getOpcode(), E0);
+        til::UnaryOp(UO->getOpcode(), translate(UO->getSubExpr(), Ctx));
-  }
   }
   return new (Arena) til::Undefined(UO);
 }
@@ -232,16 +231,15 @@ til::SExpr *SExprBuilder::translateBinaryOperator(const BinaryOperator *BO,
   case BO_Xor:
   case BO_Or:
   case BO_LAnd:
-  case BO_LOr: {
+  case BO_LOr:
-    til::SExpr *E0 = translate(BO->getLHS(), Ctx);
+    return new (Arena)
-    til::SExpr *E1 = translate(BO->getRHS(), Ctx);
+        til::BinaryOp(BO->getOpcode(), translate(BO->getLHS(), Ctx),
-    return new (Arena) til::BinaryOp(BO->getOpcode(), E0, E1);
+                      translate(BO->getRHS(), Ctx));
-  }
+
-  case BO_Assign: {
+  case BO_Assign:
-    til::SExpr *E0 = translate(BO->getLHS(), Ctx);
+    return new (Arena)
-    til::SExpr *E1 = translate(BO->getRHS(), Ctx);
+        til::Store(translate(BO->getLHS(), Ctx), translate(BO->getRHS(), Ctx));
-    return new (Arena) til::Store(E0, E1);
+
-  }
   case BO_MulAssign:
   case BO_DivAssign:
   case BO_RemAssign:
@@ -284,42 +282,39 @@ til::SExpr *SExprBuilder::translateCastExpr(const CastExpr *CE,
   }
 }
 
-
+til::SExpr *
-til::SExpr *SExprBuilder::translateArraySubscriptExpr(
+SExprBuilder::translateArraySubscriptExpr(const ArraySubscriptExpr *E,
-    const ArraySubscriptExpr *E, CallingContext *Ctx) {
+                                          CallingContext *Ctx) {
   return new (Arena) til::Undefined(E);
 }
 
-
+til::SExpr *
-til::SExpr *SExprBuilder::translateConditionalOperator(
+SExprBuilder::translateConditionalOperator(const ConditionalOperator *C,
-    const ConditionalOperator *C,  CallingContext *Ctx) {
+                                           CallingContext *Ctx) {
   return new (Arena) til::Undefined(C);
 }
 
-
 til::SExpr *SExprBuilder::translateBinaryConditionalOperator(
     const BinaryConditionalOperator *C, CallingContext *Ctx) {
   return new (Arena) til::Undefined(C);
 }
 
-
 // Build a complete SCFG from a clang CFG.
-class SCFGBuilder : public CFGVisitor {
+class SCFGBuilder {
-public:
+  void addStatement(til::SExpr* E, const Stmt *S) {
-  til::Variable *addStatement(til::SExpr* E, const Stmt *S) {
     if (!E)
-      return 0;
+      return;
     if (til::ThreadSafetyTIL::isTrivial(E))
-      return 0;
+      return;
 
     til::Variable *V = new (Arena) til::Variable(til::Variable::VK_Let, E);
     V->setID(CurrentBlockID, CurrentVarID++);
     CurrentBB->addInstr(V);
     if (S)
       BuildEx.insertStmt(S, V);
-    return V;
   }
 
+public:
   // Enter the CFG for Decl D, and perform any initial setup operations.
   void enterCFG(CFG *Cfg, const NamedDecl *D, const CFGBlock *First) {
     Scfg = new (Arena) til::SCFG(Arena, Cfg->getNumBlockIDs());
@@ -345,7 +340,7 @@ public:
     til::SExpr *Sf = new (Arena) til::LiteralPtr(VD);
     til::SExpr *Dr = new (Arena) til::LiteralPtr(DD);
     til::SExpr *Ap = new (Arena) til::Apply(Dr, Sf);
-    til::SExpr *E = new (Arena) til::Call(Ap, 0);
+    til::SExpr *E = new (Arena) til::Call(Ap);
     addStatement(E, nullptr);
   }
 
@@ -363,20 +358,15 @@ public:
 
   // Leave the CFG, and perform any final cleanup operations.
   void exitCFG(const CFGBlock *Last) {
-    if (CallCtx) {
+    delete CallCtx;
-      delete CallCtx;
+    CallCtx = nullptr;
-      CallCtx = nullptr;
-    }
   }
 
   SCFGBuilder(til::MemRegionRef A)
-      : Arena(A), Scfg(0), CurrentBB(0), CurrentBlockID(0),
+      : Arena(A), Scfg(nullptr), CurrentBB(nullptr), CurrentBlockID(0),
-        CallCtx(0), SMap(new SExprBuilder::StatementMap()),
+        CurrentVarID(0), CallCtx(nullptr),
-        BuildEx(A, SMap)
+        SMap(new SExprBuilder::StatementMap()), BuildEx(A, SMap) {}
-  { }
+  ~SCFGBuilder() { delete SMap; }
-  ~SCFGBuilder() {
-    delete SMap;
-  }
 
   til::SCFG *getCFG() const { return Scfg; }