<div class="gmail_quote">On Mon, Oct 17, 2011 at 2:33 PM, DeLesley Hutchins <span dir="ltr"><<a href="mailto:delesley@google.com">delesley@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Author: delesley<br>
Date: Mon Oct 17 16:33:35 2011<br>
New Revision: 142260<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=142260&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=142260&view=rev</a><br>
Log:<br>
Substitute for arguments in method calls -- refactoring<br></blockquote><div><br></div><div>Can you clarify what's going on here? Is this a zero-functionality refactoring? What's the movitation?</div><div><br></div>
<div>If it has a functionality change, it should have a testcase update..</div><div><br></div><div>A few initial comments inline...</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Modified:<br>
cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h<br>
cfe/trunk/lib/Analysis/ThreadSafety.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h?rev=142260&r1=142259&r2=142260&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h?rev=142260&r1=142259&r2=142260&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h (original)<br>
+++ cfe/trunk/include/clang/Analysis/Analyses/ThreadSafety.h Mon Oct 17 16:33:35 2011<br>
@@ -67,7 +67,7 @@<br>
class ThreadSafetyHandler {<br>
public:<br>
typedef llvm::StringRef Name;<br>
- virtual ~ThreadSafetyHandler() = 0;<br>
+ virtual ~ThreadSafetyHandler();<br></blockquote><div><br></div><div>Why is this changing? ThreadSafetyHandler should be abstract?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
/// Warn about lock expressions which fail to resolve to lockable objects.<br>
/// \param Loc -- the SourceLocation of the unresolved expression.<br>
<br>
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=142260&r1=142259&r2=142260&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=142260&r1=142259&r2=142260&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)<br>
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Mon Oct 17 16:33:35 2011<br>
@@ -40,15 +40,6 @@<br>
// Key method definition<br>
ThreadSafetyHandler::~ThreadSafetyHandler() {}<br>
<br>
-// Helper function<br>
-static Expr *getParent(Expr *Exp) {<br>
- if (MemberExpr *ME = dyn_cast<MemberExpr>(Exp))<br>
- return ME->getBase();<br>
- if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Exp))<br>
- return CE->getImplicitObjectArgument();<br>
- return 0;<br>
-}<br>
-<br>
namespace {<br>
/// \brief Implements a set of CFGBlocks using a BitVector.<br>
///<br>
@@ -179,19 +170,51 @@<br>
if (Parent)<br>
buildMutexID(Parent, 0);<br>
else<br>
- return; // mutexID is still valid in this case<br>
+ return; // mutexID is still valid in this case<br></blockquote><div><br></div><div>There are a lot of whitespace, line break, and comment changes here. Please separate out purely stylistic updates from refactorings (as well as refactorings from functional changes) when they touch separate areas of code. (Clearly it's fine to fix style in the code you're already touching during a refactoring.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
} else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))<br>
buildMutexID(CE->getSubExpr(), Parent);<br>
else<br>
- DeclSeq.clear(); // invalid lock expression<br>
+ DeclSeq.clear(); // Mark as invalid lock expression.<br>
+ }<br>
+<br>
+ /// \brief Construct a MutexID from an expression.<br>
+ /// \param MutexExp The original mutex expression within an attribute<br>
+ /// \param DeclExp An expression involving the Decl on which the attribute<br>
+ /// occurs.<br>
+ /// \param D The declaration to which the lock/unlock attribute is attached.<br>
+ void buildMutexIDFromExp(Expr *MutexExp, Expr *DeclExp, const NamedDecl *D) {<br>
+ Expr *Parent = 0;<br>
+<br>
+ if (DeclExp == 0) {<br>
+ buildMutexID(MutexExp, 0);<br>
+ return;<br>
+ }<br>
+<br>
+ if (MemberExpr *ME = dyn_cast<MemberExpr>(DeclExp))<br>
+ Parent = ME->getBase();<br>
+ else if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(DeclExp))<br>
+ Parent = CE->getImplicitObjectArgument();<br>
+<br>
+ // If the attribute has no arguments, then assume the argument is "this".<br>
+ if (MutexExp == 0) {<br>
+ buildMutexID(Parent, 0);<br>
+ return;<br>
+ }<br>
+ buildMutexID(MutexExp, Parent);<br>
}<br>
<br>
public:<br>
- MutexID(Expr *LExpr, Expr *ParentExpr) {<br>
- buildMutexID(LExpr, ParentExpr);<br>
+ /// \param MutexExp The original mutex expression within an attribute<br>
+ /// \param DeclExp An expression involving the Decl on which the attribute<br>
+ /// occurs.<br>
+ /// \param D The declaration to which the lock/unlock attribute is attached.<br>
+ /// Caller must check isValid() after construction.<br>
+ MutexID(Expr* MutexExp, Expr *DeclExp, const NamedDecl* D) {<br>
+ buildMutexIDFromExp(MutexExp, DeclExp, D);<br>
}<br>
<br>
- /// If we encounter part of a lock expression we cannot parse<br>
+ /// Return true if this is a valid decl sequence.<br>
+ /// Caller must call this by hand after construction to handle errors.<br>
bool isValid() const {<br>
return !DeclSeq.empty();<br>
}<br>
@@ -278,9 +301,8 @@<br>
Lockset::Factory &LocksetFactory;<br>
<br>
// Helper functions<br>
- void removeLock(SourceLocation UnlockLoc, Expr *LockExp, Expr *Parent);<br>
- void addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,<br>
- LockKind LK);<br>
+ void removeLock(SourceLocation UnlockLoc, MutexID &Mutex);<br>
+ void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK);<br>
const ValueDecl *getValueDecl(Expr *Exp);<br>
void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,<br>
Expr *MutexExp, ProtectedOperationKind POK);<br>
@@ -288,7 +310,8 @@<br>
void checkDereference(Expr *Exp, AccessKind AK);<br>
<br>
template <class AttrType><br>
- void addLocksToSet(LockKind LK, Attr *Attr, CXXMemberCallExpr *Exp);<br>
+ void addLocksToSet(LockKind LK, AttrType *Attr, CXXMemberCallExpr *Exp,<br>
+ NamedDecl* D);<br>
<br>
/// \brief Returns true if the lockset contains a lock, regardless of whether<br>
/// the lock is held exclusively or shared.<br>
@@ -335,16 +358,10 @@<br>
/// \brief Add a new lock to the lockset, warning if the lock is already there.<br>
/// \param LockLoc The source location of the acquire<br>
/// \param LockExp The lock expression corresponding to the lock to be added<br>
-void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr *Parent,<br>
+void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex,<br>
LockKind LK) {<br>
// FIXME: deal with acquired before/after annotations. We can write a first<br>
// pass that does the transitive lookup lazily, and refine afterwards.<br>
- MutexID Mutex(LockExp, Parent);<br>
- if (!Mutex.isValid()) {<br>
- Handler.handleInvalidLockExp(LockExp->getExprLoc());<br>
- return;<br>
- }<br>
-<br>
LockData NewLock(LockLoc, LK);<br>
<br>
// FIXME: Don't always warn when we have support for reentrant locks.<br>
@@ -356,14 +373,7 @@<br>
/// \brief Remove a lock from the lockset, warning if the lock is not there.<br>
/// \param LockExp The lock expression corresponding to the lock to be removed<br>
/// \param UnlockLoc The source location of the unlock (only used in error msg)<br>
-void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp,<br>
- Expr *Parent) {<br>
- MutexID Mutex(LockExp, Parent);<br>
- if (!Mutex.isValid()) {<br>
- Handler.handleInvalidLockExp(LockExp->getExprLoc());<br>
- return;<br>
- }<br>
-<br>
+void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID &Mutex) {<br>
Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);<br>
if(NewLSet == LSet)<br>
Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);<br>
@@ -383,13 +393,13 @@<br>
}<br>
<br>
/// \brief Warn if the LSet does not contain a lock sufficient to protect access<br>
-/// of at least the passed in AccessType.<br>
+/// of at least the passed in AccessKind.<br>
void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,<br>
AccessKind AK, Expr *MutexExp,<br>
ProtectedOperationKind POK) {<br>
LockKind LK = getLockKindFromAccessKind(AK);<br>
- Expr *Parent = getParent(Exp);<br>
- MutexID Mutex(MutexExp, Parent);<br>
+<br>
+ MutexID Mutex(MutexExp, Exp, D);<br>
if (!Mutex.isValid())<br>
Handler.handleInvalidLockExp(MutexExp->getExprLoc());<br>
else if (!locksetContainsAtLeast(Mutex, LK))<br>
@@ -484,22 +494,31 @@<br>
/// \brief This function, parameterized by an attribute type, is used to add a<br>
/// set of locks specified as attribute arguments to the lockset.<br>
template <typename AttrType><br>
-void BuildLockset::addLocksToSet(LockKind LK, Attr *Attr,<br>
- CXXMemberCallExpr *Exp) {<br>
+void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,<br>
+ CXXMemberCallExpr *Exp,<br>
+ NamedDecl* FunDecl) {<br>
typedef typename AttrType::args_iterator iterator_type;<br>
+<br>
SourceLocation ExpLocation = Exp->getExprLoc();<br>
- Expr *Parent = Exp->getImplicitObjectArgument();<br>
- AttrType *SpecificAttr = cast<AttrType>(Attr);<br>
<br>
- if (SpecificAttr->args_size() == 0) {<br>
+ if (Attr->args_size() == 0) {<br>
// The mutex held is the "this" object.<br>
- addLock(ExpLocation, Parent, 0, LK);<br>
+<br>
+ MutexID Mutex(0, Exp, FunDecl);<br>
+ if (!Mutex.isValid())<br>
+ Handler.handleInvalidLockExp(Exp->getExprLoc());<br>
+ else<br>
+ addLock(ExpLocation, Mutex, LK);<br>
return;<br>
}<br>
<br>
- for (iterator_type I = SpecificAttr->args_begin(),<br>
- E = SpecificAttr->args_end(); I != E; ++I)<br>
- addLock(ExpLocation, *I, Parent, LK);<br>
+ for (iterator_type I=Attr->args_begin(), E=Attr->args_end(); I != E; ++I) {<br>
+ MutexID Mutex(*I, Exp, FunDecl);<br>
+ if (!Mutex.isValid())<br>
+ Handler.handleInvalidLockExp(Exp->getExprLoc());<br>
+ else<br>
+ addLock(ExpLocation, Mutex, LK);<br>
+ }<br>
}<br>
<br>
/// \brief When visiting CXXMemberCallExprs we need to examine the attributes on<br>
@@ -516,11 +535,9 @@<br>
/// FIXME: Do not flag an error for member variables accessed in constructors/<br>
/// destructors<br>
void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) {<br>
- NamedDecl *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());<br>
-<br>
SourceLocation ExpLocation = Exp->getExprLoc();<br>
- Expr *Parent = Exp->getImplicitObjectArgument();<br>
<br>
+ NamedDecl *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());<br>
if(!D || !D->hasAttrs())<br>
return;<br>
<br>
@@ -530,15 +547,19 @@<br>
switch (Attr->getKind()) {<br>
// When we encounter an exclusive lock function, we need to add the lock<br>
// to our lockset with kind exclusive.<br>
- case attr::ExclusiveLockFunction:<br>
- addLocksToSet<ExclusiveLockFunctionAttr>(LK_Exclusive, Attr, Exp);<br>
+ case attr::ExclusiveLockFunction: {<br>
+ ExclusiveLockFunctionAttr *A = cast<ExclusiveLockFunctionAttr>(Attr);<br>
+ addLocksToSet(LK_Exclusive, A, Exp, D);<br>
break;<br>
+ }<br>
<br>
// When we encounter a shared lock function, we need to add the lock<br>
// to our lockset with kind shared.<br>
- case attr::SharedLockFunction:<br>
- addLocksToSet<SharedLockFunctionAttr>(LK_Shared, Attr, Exp);<br>
+ case attr::SharedLockFunction: {<br>
+ SharedLockFunctionAttr *A = cast<SharedLockFunctionAttr>(Attr);<br>
+ addLocksToSet(LK_Shared, A, Exp, D);<br>
break;<br>
+ }<br>
<br>
// When we encounter an unlock function, we need to remove unlocked<br>
// mutexes from the lockset, and flag a warning if they are not there.<br>
@@ -546,13 +567,22 @@<br>
UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);<br>
<br>
if (UFAttr->args_size() == 0) { // The lock held is the "this" object.<br>
- removeLock(ExpLocation, Parent, 0);<br>
+ MutexID Mu(0, Exp, D);<br>
+ if (!Mu.isValid())<br>
+ Handler.handleInvalidLockExp(Exp->getExprLoc());<br>
+ else<br>
+ removeLock(ExpLocation, Mu);<br>
break;<br>
}<br>
<br>
for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),<br>
- E = UFAttr->args_end(); I != E; ++I)<br>
- removeLock(ExpLocation, *I, Parent);<br>
+ E = UFAttr->args_end(); I != E; ++I) {<br>
+ MutexID Mutex(*I, Exp, D);<br>
+ if (!Mutex.isValid())<br>
+ Handler.handleInvalidLockExp(Exp->getExprLoc());<br>
+ else<br>
+ removeLock(ExpLocation, Mutex);<br>
+ }<br>
break;<br>
}<br>
<br>
@@ -579,7 +609,7 @@<br>
LocksExcludedAttr *LEAttr = cast<LocksExcludedAttr>(Attr);<br>
for (LocksExcludedAttr::args_iterator I = LEAttr->args_begin(),<br>
E = LEAttr->args_end(); I != E; ++I) {<br>
- MutexID Mutex(*I, Parent);<br>
+ MutexID Mutex(*I, Exp, D);<br>
if (!Mutex.isValid())<br>
Handler.handleInvalidLockExp((*I)->getExprLoc());<br>
else if (locksetContains(Mutex))<br>
@@ -641,11 +671,11 @@<br>
<br>
static Lockset addLock(ThreadSafetyHandler &Handler,<br>
Lockset::Factory &LocksetFactory,<br>
- Lockset &LSet, Expr *LockExp, LockKind LK,<br>
- SourceLocation Loc) {<br>
- MutexID Mutex(LockExp, 0);<br>
+ Lockset &LSet, Expr *MutexExp, const NamedDecl *D,<br>
+ LockKind LK, SourceLocation Loc) {<br>
+ MutexID Mutex(MutexExp, 0, D);<br>
if (!Mutex.isValid()) {<br>
- Handler.handleInvalidLockExp(LockExp->getExprLoc());<br>
+ Handler.handleInvalidLockExp(MutexExp->getExprLoc());<br>
return LSet;<br>
}<br>
LockData NewLock(Loc, LK);<br>
@@ -663,8 +693,12 @@<br>
ThreadSafetyHandler &Handler) {<br>
CFG *CFGraph = AC.getCFG();<br>
if (!CFGraph) return;<br>
- const Decl *D = AC.getDecl();<br>
- if (D && D->getAttr<NoThreadSafetyAnalysisAttr>()) return;<br>
+ const NamedDecl *D = dyn_cast_or_null<NamedDecl>(AC.getDecl());<br>
+<br>
+ if (!D)<br>
+ return; // Ignore anonymous functions for now.<br>
+ if (D->getAttr<NoThreadSafetyAnalysisAttr>())<br>
+ return;<br>
<br>
Lockset::Factory LocksetFactory;<br>
<br>
@@ -693,7 +727,7 @@<br>
SLRIter = SLRAttr->args_begin(),<br>
SLREnd = SLRAttr->args_end(); SLRIter != SLREnd; ++SLRIter)<br>
InitialLockset = addLock(Handler, LocksetFactory, InitialLockset,<br>
- *SLRIter, LK_Shared,<br>
+ *SLRIter, D, LK_Shared,<br>
AttrLoc);<br>
} else if (ExclusiveLocksRequiredAttr *ELRAttr<br>
= dyn_cast<ExclusiveLocksRequiredAttr>(Attr)) {<br>
@@ -701,7 +735,7 @@<br>
ELRIter = ELRAttr->args_begin(),<br>
ELREnd = ELRAttr->args_end(); ELRIter != ELREnd; ++ELRIter)<br>
InitialLockset = addLock(Handler, LocksetFactory, InitialLockset,<br>
- *ELRIter, LK_Exclusive,<br>
+ *ELRIter, D, LK_Exclusive,<br>
AttrLoc);<br>
}<br>
}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>