Issue 4894048: Threadsafety: Initial lockset implementation (Closed)
Patch Set 1 #
Total comments: 16
Patch Set 2 : Updated patch set that uses PO iterators #
Total comments: 156
Patch Set 3 : Merging in reviewer edits along with switch to ImmutableMap #
Total comments: 28
Patch Set 4 : Merging in more reviewer edits: release candidate #
Total comments: 32
MessagesTotal messages: 8
Sorry these comments are so late in the process. I thought i had gone through this code much more carefully before, but I hadn't. http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h File include/clang/Analysis/CFG.h (right): http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#new... include/clang/Analysis/CFG.h:800: extaneous blank line http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#new... include/clang/Analysis/CFG.h:811: ditto http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#new... include/clang/Analysis/CFG.h:822: and ditto http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:8: *************************************/ If you're going to use banners, use LLVM or Clang's standard banner format. That said, I don't think banners are really important in test files... But I know you like them. At the least, I wouldn't bother for the helper section but ::shrug:: http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:10: // more complicated classes, taken from gcc tests I'd not bother with this comment. it also seems weirdly out of context here. http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:52: // sls_mw.x = 5; // turn mu into sls_mw.mu FIXME? http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:114: else { Consistent else style? http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:131: Destructable d; // same behaviour without this line This comment doesn't have enough context for me to understand it, or the code... http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:144: expected-warning {{unlocking 'sls_mu' that was not acquired}} When using the continuation of the comment line, please keep the '//' prefix on every line. That way the reader knows at a glance that these are still comments. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; Maybe 'VisitedBlockIDs' http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:608: CFGBlockSet(const CFGBlockSet &S) : BitSet(S.BitSet) {} Is this necessary? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) { Why call this one count? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:633: /// iterator to use CFGBlockSets. It would be good to comment above on CFGBlockSet that this is an adaptor designed specifically to allow usage in po_iterator_storage. Otherwise, the somewhat strange API doesn't make a lot of sense. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety This comment seems weird. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator; dfpo? Whynot just 'po_iterator? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) { maybe just 'Graph' as the name? Also, why isn't this just inlined into the constructor? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet), Does this copy the BSet? You create a CFGBlockSet member in the po_storage above, I expected that to provide all the storage necessary. Why do we need a local variable? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:687: typedef llvm::SmallVector<DelayedDiag, 4> DiagList; I would move all the code related to delayed diagnostic emission down to be right above the function that actually needs it. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:706: S.Diag((*I).first, (*I).second); -> http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:709: extraneous newline. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:710: /// Thread-safety analysis works by comparing lock expressions. Within the Need a \brief bit in the comment. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:732: /// Note we perform substitution on free variables such as "this" when This comment doesn't really make sense for me... where did 'x' come from? Your example below makes lots of sense, its the paragraph of text that doesn't necessarily seem to agree with it. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock { Why is this called 'Lock'? That seems like an overly generic name for this class. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:740: Expr *LockExp; // currently used for debugging Can you nuke this now? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:749: Lock(SourceLocation Acq, Expr *LExpr) : LockExp(LExpr), AcquireLoc(Acq) { It seems odd that the parameter order is the reverse from ethe member order. Also, maybe "Loc" instead of "Acq"? Or better, use the exact name of the members. That works in initializer lists. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:757: } Is this necessary? I thought you removed it. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:771: // output by SourceLocation when iterating through this lockset. This last sentence doesn't parse. Specifically, its not a "However" its a "Consequentially", but maybe it can just be reworded to be simpler. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: } Another method I thought You removed.. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:790: /// is a way to get simple, and consistent, lock names. Is there a reason not to store the text that makes up the expression itself? Might be worth at least a comment. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:795: // currently only used for debugging Again, can we remove it then? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:820: llvm_unreachable("Expected lock expression!"); // FIXME: add diagnostic unreachable likely isn't appropriate here. report_fatal_error or some such. (or a diagnostic) http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> { Do we still want this visitor here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:868: S.Diag(Lock.getLoc(), diag::warn_double_unlock) << Lock.getName(); This isn't necessarily a double unlock. It's quite likely that they're unlocking something that has never been locked. We should diagnose it as an unlock without a matching lock. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:874: // FIXME: checking for guarded_by/var and pt_guarded_by/var Maybe omit this visit method and move the fixme into the class? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:889: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) { just 'unsigned' http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:891: switch(Attr->getKind()){ space before {!!!! ;] http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:892: No need for the blank line.. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF = Can't we get something about Attr in here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:897: dyn_cast<ExclusiveLockFunctionAttr>(Attr); This should be a cast, not a dyn_cast. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent); We should break after this, no? We don't need to go into the loop. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:904: } no need for curlies. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:911: // from the lockset (they better be in there!) Maybe mention that we will actually diagnose if they aren't in the lockset? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:914: dyn_cast<UnlockFunctionAttr>(ArgAttrs[i]); cast< http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:916: if (UF->args_size() == 0) // lock held is the "this" object Complete sentences in comments please. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:917: RemoveLock(ExpLocation, Parent); Again, break. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules Huh? This comment seems stale. Move it into the switch? You can add cases and then comment before the berak. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:937: /// issue warnings for any locks that are not contained in the intersection. Can you be a bit more clear here? Clearly you're not warning on every lock in the program not contained in the intersection. This doesn't appear to merge anything. It's just warning about the symmetric difference of the two sets, and computing the intersection. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:938: Lockset mergeAndWarn(Sema &S, Lockset LSet1, Lockset LSet2, This should be a static function. In general, use anonymous namespaces for types, and static keyword for functions. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:943: for (Lockset::iterator I = LSet2.begin(), E = LSet2.end(); Does this really not fit on a single line? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:952: for (Lockset::iterator I = LSet1.begin(), E = LSet1.end(); Does this really not fit on a single line? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:958: Intersection = Fact.remove(Intersection, *I); I wonder if it would be more efficient to build the intersection once rather than removing these. Essentially, we could keep a small vector and add entries to it when they are in the intersection, and then return an immutable set from that. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:976: /// Compare this lockset with the loopEndSet, and output a warning if the two For your comments, please always add a \brief line at the start, and use the \param tag to marke comments for specific parameters. I think you can do that in a followup change, but its important to do. Which lockset is "this" lockset? I think this comment is just stale. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:981: const Lockset PreLoopSet, This argument order and naming confuses me. Lets get the PreLoopSet first, followed by the LoopEndSet, and name them consistently. I'me fine with either Loop{Start,End}Set or with {Pre,Post}LoopSet. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:986: // Warn for locks held at the end of the loop, but not at the start. This seems backwards. Why not first exampine PReLoopSet, and then LoopEndSet? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1002: // first statement location, fall back on the current location. Is this just because we don't track the location of the releases of locks? I wonder if it would be worth doing that to make diagnosticts better. At least worth a comment. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1004: FirstLocInLoop = I->getLoc(); Why would this ever happen? Why can't we insist on always having a location? It seems really unfortunate to diagnose this on a random lock acquisition long before the loop that causes a problem. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1019: CFG *Cfg = AC.getCFG(); 'Graph' instead of 'Cfg' or some such? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1030: std::vector<Lockset> EntryLocksets(Cfg->getNumBlockIDs(), I'd like to leave some FIXMEs around to try and improve the performance impact of this. As it stands, this will be a very expensive analysis for large CFG functions. At the very least, we'll almost certainly want to have SmallVectors for these to optimize small CFGs, but all of that can take the form of followup patches. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1035: TopologicallySortedCFG tsCFG(Cfg); SortedGraph maybe? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1040: Extraneous newline. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1041: // get the next block and process it This comment doesn't really add any value. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1042: const CFGBlock *CurrBlock = (*I); Extraneous parens. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1047: // Create a default initial lockset in case there are no predecessors. Why not only do this when !LocksetInitialized after your loop below? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1065: PE = CurrBlock->pred_end(); PrevBlockIter != PE; ++PrevBlockIter) { Either use PI and PE, or use full names for both. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block. This is clear from the code. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1090: Lockset Exitset = LocksetBuilder.getLockset(); Is copying these free? Because otherwise we're making this way more expensive than it needs to be. Maybe you just want to store a reference here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset; Why can't we make Entryset a reference to this up above, and build it in-place? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1115: if (!FinalLockset.isEmpty()) { // locks still held at end of CFG! This comment doesn't add anything. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:278: SmallVectorImpl<Expr*> &outArgs, Just "Args" is fine. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:381: SmallVector<Expr*, 1> ArgVector; FIXME to actually use the argcs? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:616: SmallVector<Expr*, 1> Args; FIXME to actually use the args? Also it'd be nice to consistently use Args or ArgVector
Sign in to reply to this message.
I made various small edits throughout the patch. Let me know if I can commit it today. DeLesley, do you see the comment for you in the topological sort section of AnalysisBasedWarnings? Cheers, Caitlin http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h File include/clang/Analysis/CFG.h (right): http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#new... include/clang/Analysis/CFG.h:800: This section is deleted in the later patch. On 2011/08/19 23:54:06, chandlerc wrote: > extaneous blank line http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:8: *************************************/ Changed to llvm style. On 2011/08/19 23:54:06, chandlerc wrote: > If you're going to use banners, use LLVM or Clang's standard banner format. > > That said, I don't think banners are really important in test files... But I > know you like them. At the least, I wouldn't bother for the helper section but > ::shrug:: http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:10: // more complicated classes, taken from gcc tests Deleted comment. On 2011/08/19 23:54:06, chandlerc wrote: > I'd not bother with this comment. it also seems weirdly out of context here. http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:52: // sls_mw.x = 5; // turn mu into sls_mw.mu On 2011/08/19 23:54:06, chandlerc wrote: > FIXME? Done. http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:114: else { On 2011/08/19 23:54:06, chandlerc wrote: > Consistent else style? Done. http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:131: Destructable d; // same behaviour without this line Got rid of "Destructable" On 2011/08/19 23:54:06, chandlerc wrote: > This comment doesn't have enough context for me to understand it, or the code... Done. http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:144: expected-warning {{unlocking 'sls_mu' that was not acquired}} I think in the context of "expected-warning" comments that actually makes it more confusing since only having one '//' prefix makes the line for which the warning applies clear. Also, this style is consistent with the previously committed test file. On 2011/08/19 23:54:06, chandlerc wrote: > When using the continuation of the comment line, please keep the '//' prefix on > every line. That way the reader knows at a glance that these are still comments. http://codereview.appspot.com/4894048/diff/2001/include/clang/Analysis/CFG.h File include/clang/Analysis/CFG.h (right): http://codereview.appspot.com/4894048/diff/2001/include/clang/Analysis/CFG.h#... include/clang/Analysis/CFG.h:799: Deleted this change. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; On 2011/08/19 23:54:06, chandlerc wrote: > Maybe 'VisitedBlockIDs' Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:608: CFGBlockSet(const CFGBlockSet &S) : BitSet(S.BitSet) {} Deleted. On 2011/08/19 23:54:06, chandlerc wrote: > Is this necessary? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) { Removed. On 2011/08/19 23:54:06, chandlerc wrote: > Why call this one count? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:633: /// iterator to use CFGBlockSets. On 2011/08/19 23:54:06, chandlerc wrote: > It would be good to comment above on CFGBlockSet that this is an adaptor > designed specifically to allow usage in po_iterator_storage. Otherwise, the > somewhat strange API doesn't make a lot of sense. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety changed On 2011/08/19 23:54:06, chandlerc wrote: > This comment seems weird. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator; Changed. On 2011/08/19 23:54:06, chandlerc wrote: > dfpo? Whynot just 'po_iterator? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) { Changed to CFGraph and inlined. On 2011/08/19 23:54:06, chandlerc wrote: > maybe just 'Graph' as the name? > > Also, why isn't this just inlined into the constructor? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet), I think we need to pass a CFGBlockSet into the constructor for po_storage. DeLeseley, any additional comments on this? On 2011/08/19 23:54:06, chandlerc wrote: > Does this copy the BSet? You create a CFGBlockSet member in the po_storage > above, I expected that to provide all the storage necessary. Why do we need a > local variable? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:687: typedef llvm::SmallVector<DelayedDiag, 4> DiagList; On 2011/08/19 23:54:06, chandlerc wrote: > I would move all the code related to delayed diagnostic emission down to be > right above the function that actually needs it. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:706: S.Diag((*I).first, (*I).second); On 2011/08/19 23:54:06, chandlerc wrote: > -> Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:709: On 2011/08/19 23:54:06, chandlerc wrote: > extraneous newline. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:710: /// Thread-safety analysis works by comparing lock expressions. Within the On 2011/08/19 23:54:06, chandlerc wrote: > Need a \brief bit in the comment. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:732: /// Note we perform substitution on free variables such as "this" when Moved below example and clarified. On 2011/08/19 23:54:06, chandlerc wrote: > This comment doesn't really make sense for me... where did 'x' come from? > > Your example below makes lots of sense, its the paragraph of text that doesn't > necessarily seem to agree with it. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock { I think it is short, to the point, and makes sense in this context. We track a set (a lockset) of what locks we have currently acquired. It makes sense to me to call it a lockset and to call the things that go into it locks. Do you have another suggestion? On 2011/08/19 23:54:06, chandlerc wrote: > Why is this called 'Lock'? That seems like an overly generic name for this > class. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:740: Expr *LockExp; // currently used for debugging On 2011/08/19 23:54:06, chandlerc wrote: > Can you nuke this now? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:749: Lock(SourceLocation Acq, Expr *LExpr) : LockExp(LExpr), AcquireLoc(Acq) { Changed to Loc On 2011/08/19 23:54:06, chandlerc wrote: > It seems odd that the parameter order is the reverse from ethe member order. > > Also, maybe "Loc" instead of "Acq"? Or better, use the exact name of the > members. That works in initializer lists. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:757: } I did, in the patch that refactors to use an ImmutableMap. I merged the two patches together so we can commit all the edits at once. On 2011/08/19 23:54:06, chandlerc wrote: > Is this necessary? I thought you removed it. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:771: // output by SourceLocation when iterating through this lockset. Removed the "however" On 2011/08/19 23:54:06, chandlerc wrote: > This last sentence doesn't parse. Specifically, its not a "However" its a > "Consequentially", but maybe it can just be reworded to be simpler. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: } I did, in the patch that refactors to use an ImmutableMap. I merged the two patches together so we can commit all the edits at once. On 2011/08/19 23:54:06, chandlerc wrote: > Another method I thought You removed.. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:790: /// is a way to get simple, and consistent, lock names. Added a comment that it is for security reasons. Is there anything else I should say? On 2011/08/19 23:54:06, chandlerc wrote: > Is there a reason not to store the text that makes up the expression itself? > Might be worth at least a comment. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:795: // currently only used for debugging removed. On 2011/08/19 23:54:06, chandlerc wrote: > Again, can we remove it then? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:820: llvm_unreachable("Expected lock expression!"); // FIXME: add diagnostic On 2011/08/19 23:54:06, chandlerc wrote: > unreachable likely isn't appropriate here. report_fatal_error or some such. (or > a diagnostic) Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> { I think we do for now. It definitely makes it easier to structure. I will add a fixme to revisit this question later. On 2011/08/19 23:54:06, chandlerc wrote: > Do we still want this visitor here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:868: S.Diag(Lock.getLoc(), diag::warn_double_unlock) << Lock.getName(); Changed text of warning title. On 2011/08/19 23:54:06, chandlerc wrote: > This isn't necessarily a double unlock. It's quite likely that they're unlocking > something that has never been locked. We should diagnose it as an unlock without > a matching lock. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:874: // FIXME: checking for guarded_by/var and pt_guarded_by/var Patch that fixes this is already under review (please review it). Would rather keep like this and just submit the next patch as soon as possible. On 2011/08/19 23:54:06, chandlerc wrote: > Maybe omit this visit method and move the fixme into the class? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:889: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) { On 2011/08/19 23:54:06, chandlerc wrote: > just 'unsigned' Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:891: switch(Attr->getKind()){ On 2011/08/19 23:54:06, chandlerc wrote: > space before {!!!! ;] Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:892: I would like to keep one blank line before each case. On 2011/08/19 23:54:06, chandlerc wrote: > No need for the blank line.. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF = ? On 2011/08/19 23:54:06, chandlerc wrote: > Can't we get something about Attr in here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:897: dyn_cast<ExclusiveLockFunctionAttr>(Attr); On 2011/08/19 23:54:06, chandlerc wrote: > This should be a cast, not a dyn_cast. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent); In that case args_begin() == args_end(), so we will not execute any iterations of the loop. On 2011/08/19 23:54:06, chandlerc wrote: > We should break after this, no? We don't need to go into the loop. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:904: } On 2011/08/19 23:54:06, chandlerc wrote: > no need for curlies. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:911: // from the lockset (they better be in there!) On 2011/08/19 23:54:06, chandlerc wrote: > Maybe mention that we will actually diagnose if they aren't in the lockset? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:914: dyn_cast<UnlockFunctionAttr>(ArgAttrs[i]); On 2011/08/19 23:54:06, chandlerc wrote: > cast< Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:916: if (UF->args_size() == 0) // lock held is the "this" object On 2011/08/19 23:54:06, chandlerc wrote: > Complete sentences in comments please. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:917: RemoveLock(ExpLocation, Parent); On 2011/08/19 23:54:06, chandlerc wrote: > Again, break. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules Moved. The next patch adds cases. On 2011/08/19 23:54:06, chandlerc wrote: > Huh? This comment seems stale. Move it into the switch? You can add cases and > then comment before the berak. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:937: /// issue warnings for any locks that are not contained in the intersection. Changed to intersectAndWarn. Added more comments. On 2011/08/19 23:54:06, chandlerc wrote: > Can you be a bit more clear here? Clearly you're not warning on every lock in > the program not contained in the intersection. > > This doesn't appear to merge anything. It's just warning about the symmetric > difference of the two sets, and computing the intersection. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:938: Lockset mergeAndWarn(Sema &S, Lockset LSet1, Lockset LSet2, On 2011/08/19 23:54:06, chandlerc wrote: > This should be a static function. > > In general, use anonymous namespaces for types, and static keyword for > functions. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:943: for (Lockset::iterator I = LSet2.begin(), E = LSet2.end(); On 2011/08/19 23:54:06, chandlerc wrote: > Does this really not fit on a single line? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:952: for (Lockset::iterator I = LSet1.begin(), E = LSet1.end(); On 2011/08/19 23:54:06, chandlerc wrote: > Does this really not fit on a single line? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:958: Intersection = Fact.remove(Intersection, *I); I believe we looked into it and removing should be pretty efficient. Also, the common case is that there is nothing to remove (no error). Also, I think we would still have to add one element at a time. On 2011/08/19 23:54:06, chandlerc wrote: > I wonder if it would be more efficient to build the intersection once rather > than removing these. Essentially, we could keep a small vector and add entries > to it when they are in the intersection, and then return an immutable set from > that. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:976: /// Compare this lockset with the loopEndSet, and output a warning if the two Fixed comment. Added brief here and other places. On 2011/08/19 23:54:06, chandlerc wrote: > For your comments, please always add a \brief line at the start, and use the > \param tag to marke comments for specific parameters. I think you can do that in > a followup change, but its important to do. > > Which lockset is "this" lockset? I think this comment is just stale. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:981: const Lockset PreLoopSet, Changed LoopEndSet to LastLoopSet. PreLoopSet is the lockset directly *before* the loop, whereas LastLoopSet is the last CFG block of the loop. I have added some clarification in the comments. On 2011/08/19 23:54:06, chandlerc wrote: > This argument order and naming confuses me. Lets get the PreLoopSet first, > followed by the LoopEndSet, and name them consistently. I'me fine with either > Loop{Start,End}Set or with {Pre,Post}LoopSet. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:986: // Warn for locks held at the end of the loop, but not at the start. Swapped these two blocks. On 2011/08/19 23:54:06, chandlerc wrote: > This seems backwards. Why not first exampine PReLoopSet, and then LoopEndSet? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1002: // first statement location, fall back on the current location. This warning is reported at the start of a loop. I have added comments to that effect. On 2011/08/19 23:54:06, chandlerc wrote: > Is this just because we don't track the location of the releases of locks? I > wonder if it would be worth doing that to make diagnosticts better. At least > worth a comment. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1004: FirstLocInLoop = I->getLoc(); I think we will always have a location, and have added an assertion to that effect. On 2011/08/19 23:54:06, chandlerc wrote: > Why would this ever happen? Why can't we insist on always having a location? It > seems really unfortunate to diagnose this on a random lock acquisition long > before the loop that causes a problem. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1019: CFG *Cfg = AC.getCFG(); Changed to CFGraph. On 2011/08/19 23:54:06, chandlerc wrote: > 'Graph' instead of 'Cfg' or some such? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1030: std::vector<Lockset> EntryLocksets(Cfg->getNumBlockIDs(), FIXME added. We could even use arrays since the size of the vectors is fixed at the number of CFG Blocks. Note that the calls to LocksetFactory.getEmptySet() all return the same thing. On 2011/08/19 23:54:06, chandlerc wrote: > I'd like to leave some FIXMEs around to try and improve the performance impact > of this. As it stands, this will be a very expensive analysis for large CFG > functions. > > At the very least, we'll almost certainly want to have SmallVectors for these to > optimize small CFGs, but all of that can take the form of followup patches. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1035: TopologicallySortedCFG tsCFG(Cfg); On 2011/08/19 23:54:06, chandlerc wrote: > SortedGraph maybe? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1040: On 2011/08/19 23:54:06, chandlerc wrote: > Extraneous newline. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1041: // get the next block and process it On 2011/08/19 23:54:06, chandlerc wrote: > This comment doesn't really add any value. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1042: const CFGBlock *CurrBlock = (*I); On 2011/08/19 23:54:06, chandlerc wrote: > Extraneous parens. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1047: // Create a default initial lockset in case there are no predecessors. There is no public default constructor for locksets. Instead, you have to set it to another lockset. getEmptySet() is very cheap since there is only one empty lockset. On 2011/08/19 23:54:06, chandlerc wrote: > Why not only do this when !LocksetInitialized after your loop below? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1065: PE = CurrBlock->pred_end(); PrevBlockIter != PE; ++PrevBlockIter) { On 2011/08/19 23:54:06, chandlerc wrote: > Either use PI and PE, or use full names for both. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block. I would still like to keep this comment. On 2011/08/19 23:54:06, chandlerc wrote: > This is clear from the code. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1090: Lockset Exitset = LocksetBuilder.getLockset(); Since we are using ImmutableMaps, I believe copying these is pretty much free. On 2011/08/19 23:54:06, chandlerc wrote: > Is copying these free? Because otherwise we're making this way more expensive > than it needs to be. > > Maybe you just want to store a reference here? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset; See above comment. On 2011/08/19 23:54:06, chandlerc wrote: > Why can't we make Entryset a reference to this up above, and build it in-place? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1115: if (!FinalLockset.isEmpty()) { // locks still held at end of CFG! Deleted. On 2011/08/19 23:54:06, chandlerc wrote: > This comment doesn't add anything. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:278: SmallVectorImpl<Expr*> &outArgs, On 2011/08/19 23:54:06, chandlerc wrote: > Just "Args" is fine. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:381: SmallVector<Expr*, 1> ArgVector; Refactored this to make it more clear that I am just checking one argument. (ArgVector was a vector for that Arg, in contrast with Args). In this case, and so the method for passing it along is different. On 2011/08/19 23:54:06, chandlerc wrote: > FIXME to actually use the argcs? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:616: SmallVector<Expr*, 1> Args; On 2011/08/19 23:54:06, chandlerc wrote: See previous response. > FIXME to actually use the args? Also it'd be nice to consistently use Args or > ArgVector
Sign in to reply to this message.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; It's supposed to implement an actual set interface for po_iterator. Thus the name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's supposed to provide. On 2011/08/22 18:30:16, supertri wrote: > On 2011/08/19 23:54:06, chandlerc wrote: > > Maybe 'VisitedBlockIDs' > > Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) { On 2011/08/19 23:54:06, chandlerc wrote: > Why call this one count? Again, it has to implement the set interface, which specifies a method called "count". I included the method for completeness; the actual template instantiations may not require it in this case. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:640: CFGBlockSet Visited; Should be CFGBlockSet &Visited. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator; On 2011/08/19 23:54:06, chandlerc wrote: > dfpo? Whynot just 'po_iterator? Because I was uncomfortable with a naming conflict between the local po_iterator and llvm::po_iterator. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) { It's not inlined, because I'm copying the RPO example code. On 2011/08/19 23:54:06, chandlerc wrote: > maybe just 'Graph' as the name? > > Also, why isn't this just inlined into the constructor? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet), See comment on line 640, above: the set in po_iterator storage should be a reference. On 2011/08/22 18:30:16, supertri wrote: > I think we need to pass a CFGBlockSet into the constructor for po_storage. > DeLeseley, any additional comments on this? > > On 2011/08/19 23:54:06, chandlerc wrote: > > Does this copy the BSet? You create a CFGBlockSet member in the po_storage > > above, I expected that to provide all the storage necessary. Why do we need a > > local variable? >
Sign in to reply to this message.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety On 2011/08/22 18:30:16, supertri wrote: > changed > > On 2011/08/19 23:54:06, chandlerc wrote: > > This comment seems weird. > I'm suggesting removing it altogether. It's clearly an anonymous namespace, and there is a big banner at the top to tell us why. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock { On 2011/08/22 18:30:16, supertri wrote: > I think it is short, to the point, and makes sense in this context. We track a > set (a lockset) of what locks we have currently acquired. It makes sense to me > to call it a lockset and to call the things that go into it locks. Do you have > another suggestion? LockSignature? I dunno. Maybe others have better names here. My problem is that this *isn't* a lock. It models a lock in the users source code by a specific set of assumed-to-be-unique aspects. I might actually call this "LockID" (note, capitalized 'D') and call the inner vector "DeclSeq" or some such. The inner construct is an implementation detail. The class itself models a way to identify a lock (not an actual lock). http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> { On 2011/08/22 18:30:16, supertri wrote: > I think we do for now. It definitely makes it easier to structure. I will add a > fixme to revisit this question later. I was really asking whether we want to use StmtVisitor rather than EvaluatedExprVisitior. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:892: On 2011/08/22 18:30:16, supertri wrote: > I would like to keep one blank line before each case. That is not the pervailing LLVM or Clang style. Consistency is the goal here. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF = On 2011/08/22 18:30:16, supertri wrote: > ? The variable name 'ELF' is both confusing (there are many things to do w/ ELF, most of them are no ExclusiveLockFunctions), and misses key information: this is an *attribute*, not the exclusive lock function itself. maybe "ExclusiveAttr"? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent); On 2011/08/22 18:30:16, supertri wrote: > In that case args_begin() == args_end(), so we will not execute any iterations > of the loop. But we shouldn't compute that the size is zero twice. What if this is a linked list, and size has linear complexity? http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules On 2011/08/22 18:30:16, supertri wrote: > Moved. The next patch adds cases. See my comment above. The request to move was specifically tied to pro-actively adding a case. I want the comment attached to some actual C++ code that will be encountered again when fixing. Currently they aren't. I'm happy attaching them to the switch itself, or to an empty (stub) case. I prefer the latter. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block. On 2011/08/22 18:30:16, supertri wrote: > I would still like to keep this comment. But why? What value does it add? I try to only add comments when there is some piece of information the reader is likely to be missing, in the hope that the comments are actually read... but fine, i'll desist.. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset; On 2011/08/22 18:30:16, supertri wrote: > See above comment. I see why we don't *need* to do this, it would seem strictly cleaner to me to make Entryset and Exitset be references to the elements of the vector / array we already have created, and mutate them in place. Even though copying relatively cheap, there is no reason to even bother in this case. http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/Diagnost... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/Diagnost... include/clang/Basic/DiagnosticSemaKinds.td:1321: def warn_unlock_but_no_acquire : Warning< At some point I think we should consistently use "lock" and "unlock" as verbs, or use "acquire" and "release" as verbs, relegating "lock" to a noun. Follow-up patch is good for this, and maybe the one switching to C++0x's naming conventions would be a good time to fix thise. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu(); Is 'this' the only variable so substituted? Essentially, I'm not sure what you mean by 'free-variables'. Are there other things (besides 'this') that are substituted? How does this work? http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:719: /// consistent with C++0x Feel free to switch immediately (in a separate patch though!) to C++0x terminology. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:785: SourceLocation AcquireLoc; Why not just map to the source location? I don't see any real indicators of future information you're expecting to add. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:812: /// \brief This is a visitor class which visits each expression in a CFGBlock. This doesn't seem to match the API. It doesn't have anything to do with a CFGBlock. Maybe just "each expression in a Stmt"? http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:897: // FIXME: shared/trylock variations This FIXME is now in a weird location. Why here? Is there some other comment we could attach it to that would make it more likely to be, well, fixed. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:924: // the lockset in deterministic order. This should be a doxyment. Also, it's not immediately obvious why this typedef is relevant to the requirement in this comment. Maybe attach the comment (and expand on it) to the EmitDiagnostics function. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:949: /// locks that are not contained in the intersection. You still need to make this more precise: "for any locks *in the union* that are not contained in the intersection." Or you can just say that you're warning on the symmetric difference, as that's how it is defined. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:998: /// a warning if the two locksets are unequal. You've repeated all of the body of this comment here. ;] Brief sections can really be brief: "Warn about different locksets in backedges of loops." or something... http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1001: /// \param LoopEndSet Locks held in the last CFG block of the loop This is using the old name. I still find these names confusing... but oh well. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1009: assert(FirstLocInLoop.isValid()); // FIXME: fail gracefully? Why FIXME? It seems like we want to always have a location, and asserting until we find all the edge cases that don't is reasonable. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1043: /// The main thread safety loop. Each block in the CFG is traversed exactly Your brief comment is longer than the rest of the comments. =/ And "The main thread safety loop" doesn't really help the reader much. Maybe as the brief: "Check a function's CFG for thread-safety violations." Then the details can follow. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1110: for (CFGBlock::const_iterator I = CurrBlock->begin(), E = CurrBlock->end(); It would seem nice to not shadow I and E here... BI, BE? http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp#ne... lib/Sema/SemaDeclAttr.cpp:274: bool checkExpLockableObj(Sema & S, Decl *D, const AttributeList & Attr, Please fix the coding conventions in this function, as commented repeatedly in prior reviews: doxyments. indentation. space after & variable names
Sign in to reply to this message.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu(); "Free variable" is a standard term used the type system literature. It includes "this", as well as any arguments to functions, e.g. l is a free variable in l->mu: foo(MyList *l) EXCLUSIVE_LOCKS_REQUIRED(l->mu) { ... } MyList *myl; foo(myl); // requires lock myl->mu to be held Notice that l is replaced with myl, in the exact same way that "this" is replaced with the target object. On 2011/08/22 21:52:12, chandlerc wrote: > Is 'this' the only variable so substituted? Essentially, I'm not sure what you > mean by 'free-variables'. > > Are there other things (besides 'this') that are substituted? How does this > work?
Sign in to reply to this message.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; On 2011/08/22 18:58:03, delesley wrote: > It's supposed to implement an actual set interface for po_iterator. Thus the > name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's > supposed to provide. The change is from BitSet to VisitedBlockIDs (not CFGBlockSet to VisitedBlockIDs). http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; On 2011/08/22 18:58:03, delesley wrote: > It's supposed to implement an actual set interface for po_iterator. Thus the > name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's > supposed to provide. The change is from BitSet to VisitedBlockIDs (not CFGBlockSet to VisitedBlockIDs). http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) { On 2011/08/22 18:58:03, delesley wrote: > On 2011/08/19 23:54:06, chandlerc wrote: > > Why call this one count? > > Again, it has to implement the set interface, which specifies a method called > "count". I included the method for completeness; the actual template > instantiations may not require it in this case. Since the "count" method was unused, it is now removed. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:640: CFGBlockSet Visited; On 2011/08/22 18:58:03, delesley wrote: > Should be CFGBlockSet &Visited. Actually, we were able to delete both of these entire templates. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > changed > > > > On 2011/08/19 23:54:06, chandlerc wrote: > > > This comment seems weird. > > > > I'm suggesting removing it altogether. It's clearly an anonymous namespace, and > there is a big banner at the top to tell us why. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator; On 2011/08/22 18:58:03, delesley wrote: > On 2011/08/19 23:54:06, chandlerc wrote: > > dfpo? Whynot just 'po_iterator? > > Because I was uncomfortable with a naming conflict between the local po_iterator > and llvm::po_iterator. ah, ok. Chandler still votes for po_iterator, so that is what we have now. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) { On 2011/08/22 18:58:03, delesley wrote: > It's not inlined, because I'm copying the RPO example code. > > On 2011/08/19 23:54:06, chandlerc wrote: > > maybe just 'Graph' as the name? > > > > Also, why isn't this just inlined into the constructor? > Ok. Changed to be inlined. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet), On 2011/08/22 18:58:03, delesley wrote: > See comment on line 640, above: the set in po_iterator storage should be a > reference. > > On 2011/08/22 18:30:16, supertri wrote: > > I think we need to pass a CFGBlockSet into the constructor for po_storage. > > DeLeseley, any additional comments on this? > > > > On 2011/08/19 23:54:06, chandlerc wrote: > > > Does this copy the BSet? You create a CFGBlockSet member in the po_storage > > > above, I expected that to provide all the storage necessary. Why do we need > a > > > local variable? > > > Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock { On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > I think it is short, to the point, and makes sense in this context. We track a > > set (a lockset) of what locks we have currently acquired. It makes sense to me > > to call it a lockset and to call the things that go into it locks. Do you have > > another suggestion? > > LockSignature? I dunno. Maybe others have better names here. My problem is that > this *isn't* a lock. It models a lock in the users source code by a specific set > of assumed-to-be-unique aspects. I might actually call this "LockID" (note, > capitalized 'D') and call the inner vector "DeclSeq" or some such. The inner > construct is an implementation detail. The class itself models a way to identify > a lock (not an actual lock). Changed to LockID and DeclSeq. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> { Yes, we do. For this patch it does not matter, the change to StmtVisitor is important when we implement the guardedby/var stuff in a subsequent patch -- and it currently lives in that patch. On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > I think we do for now. It definitely makes it easier to structure. I will add > a > > fixme to revisit this question later. > > I was really asking whether we want to use StmtVisitor rather than > EvaluatedExprVisitior. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:892: > That is not the pervailing LLVM or Clang style. Consistency is the goal here. Deleted the initial blank line. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF = On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > ? > > The variable name 'ELF' is both confusing (there are many things to do w/ ELF, > most of them are no ExclusiveLockFunctions), and misses key information: this is > an *attribute*, not the exclusive lock function itself. maybe "ExclusiveAttr"? Changed to ELFAttr. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent); On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > In that case args_begin() == args_end(), so we will not execute any iterations > > of the loop. > > But we shouldn't compute that the size is zero twice. What if this is a linked > list, and size has linear complexity? Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules > See my comment above. The request to move was specifically tied to pro-actively > adding a case. I want the comment attached to some actual C++ code that will be > encountered again when fixing. Currently they aren't. I'm happy attaching them > to the switch itself, or to an empty (stub) case. I prefer the latter. Deleted comments. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block. On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > I would still like to keep this comment. > > But why? What value does it add? I try to only add comments when there is some > piece of information the reader is likely to be missing, in the hope that the > comments are actually read... but fine, i'll desist.. Done. http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset; On 2011/08/22 21:52:12, chandlerc wrote: > On 2011/08/22 18:30:16, supertri wrote: > > See above comment. > > I see why we don't *need* to do this, it would seem strictly cleaner to me to > make Entryset and Exitset be references to the elements of the vector / array we > already have created, and mutate them in place. > > Even though copying relatively cheap, there is no reason to even bother in this > case. Done. http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/Diagnost... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/Diagnost... include/clang/Basic/DiagnosticSemaKinds.td:1321: def warn_unlock_but_no_acquire : Warning< On 2011/08/22 21:52:12, chandlerc wrote: > At some point I think we should consistently use "lock" and "unlock" as verbs, > or use "acquire" and "release" as verbs, relegating "lock" to a noun. Follow-up > patch is good for this, and maybe the one switching to C++0x's naming > conventions would be a good time to fix thise. Added Fixme. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu(); On 2011/08/22 22:02:52, delesley wrote: > "Free variable" is a standard term used the type system literature. It includes > "this", as well as any arguments to functions, e.g. l is a free variable in > l->mu: > > foo(MyList *l) EXCLUSIVE_LOCKS_REQUIRED(l->mu) { ... } > MyList *myl; > foo(myl); // requires lock myl->mu to be held > > Notice that l is replaced with myl, in the exact same way that "this" is > replaced with the target object. > > On 2011/08/22 21:52:12, chandlerc wrote: > > Is 'this' the only variable so substituted? Essentially, I'm not sure what you > > mean by 'free-variables'. > > > > Are there other things (besides 'this') that are substituted? How does this > > work? > I added a second example and removed academic terminology (as per Chandler's request). Please take a look. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:785: SourceLocation AcquireLoc; On 2011/08/22 21:52:12, chandlerc wrote: > Why not just map to the source location? I don't see any real indicators of > future information you're expecting to add. We will be adding info for shared vs. exclusive locks. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:812: /// \brief This is a visitor class which visits each expression in a CFGBlock. On 2011/08/22 21:52:12, chandlerc wrote: > This doesn't seem to match the API. It doesn't have anything to do with a > CFGBlock. Maybe just "each expression in a Stmt"? Comment changed. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:897: // FIXME: shared/trylock variations On 2011/08/22 21:52:12, chandlerc wrote: > This FIXME is now in a weird location. Why here? Is there some other comment we > could attach it to that would make it more likely to be, well, fixed. Removed. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:924: // the lockset in deterministic order. On 2011/08/22 21:52:12, chandlerc wrote: > This should be a doxyment. Also, it's not immediately obvious why this typedef > is relevant to the requirement in this comment. Maybe attach the comment (and > expand on it) to the EmitDiagnostics function. Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:949: /// locks that are not contained in the intersection. On 2011/08/22 21:52:12, chandlerc wrote: > You still need to make this more precise: "for any locks *in the union* that are > not contained in the intersection." > > Or you can just say that you're warning on the symmetric difference, as that's > how it is defined. Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:998: /// a warning if the two locksets are unequal. On 2011/08/22 21:52:12, chandlerc wrote: > You've repeated all of the body of this comment here. ;] Brief sections can > really be brief: > > "Warn about different locksets in backedges of loops." or something... Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1001: /// \param LoopEndSet Locks held in the last CFG block of the loop On 2011/08/22 21:52:12, chandlerc wrote: > This is using the old name. > > I still find these names confusing... but oh well. Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1009: assert(FirstLocInLoop.isValid()); // FIXME: fail gracefully? On 2011/08/22 21:52:12, chandlerc wrote: > Why FIXME? It seems like we want to always have a location, and asserting until > we find all the edge cases that don't is reasonable. Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1043: /// The main thread safety loop. Each block in the CFG is traversed exactly On 2011/08/22 21:52:12, chandlerc wrote: > Your brief comment is longer than the rest of the comments. =/ And "The main > thread safety loop" doesn't really help the reader much. Maybe as the brief: > "Check a function's CFG for thread-safety violations." Then the details can > follow. Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnin... lib/Sema/AnalysisBasedWarnings.cpp:1110: for (CFGBlock::const_iterator I = CurrBlock->begin(), E = CurrBlock->end(); On 2011/08/22 21:52:12, chandlerc wrote: > It would seem nice to not shadow I and E here... BI, BE? Done. http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp#ne... lib/Sema/SemaDeclAttr.cpp:274: bool checkExpLockableObj(Sema & S, Decl *D, const AttributeList & Attr, On 2011/08/22 21:52:12, chandlerc wrote: > Please fix the coding conventions in this function, as commented repeatedly in > prior reviews: > > doxyments. > indentation. > space after & > variable names Done.
Sign in to reply to this message.
Essentially cosmetic comments. Please re-write the description of whats going in to really carefully explain what you're committing here as the patch has grown. Feel free to commit once you have that and have addressed the cosmetic issues below. We can handle the rest in post-commit review. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:607: nuke blank line http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:666: /// \class Lock Stale comment. You can actually delete this line, Doxygen should pick it up by looking below. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:798: /// \class BuildLockset Probably can remove this too http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:996: const Lockset LoopEntrySet, indent is off http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1063: // Create a default initial lockset in case there are no predecessors. This is no longer quite accurate... maybe just note that we already *have* a default initial lockset. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1099: for (CFGBlock::const_iterator BI = CurrBlock->begin(), BE = CurrBlock->end(); 80 columns http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1143: Can probably nix 2 of the 4 blank lines. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:275: /// resolves to a lockable object. Join with the previous line. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:279: // Flag error if could not get record type for this argument. no 2 spaces after '//' http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:299: int Sidx = 0, Sidx? Huh? http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:301: for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) { s/unsigned int/unsigned/ http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:307: QualType Arg_QT = ArgExp->getType(); Arg_QT -> ArgTy http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:309: // Get record type. This doesn't seem to to add information reall... http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:313: // Now check if we idx into a record type function param. s/idx/index/ http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:320: uint64_t ParamIdx_from1 = ArgValue.getZExtValue(); ParamIdxFromOne? http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:321: uint64_t ParamIdx_from0 = ParamIdx_from1 - 1; ParamIdxFromZero?
Sign in to reply to this message.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:607: On 2011/08/23 18:10:04, chandlerc wrote: > nuke blank line Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:666: /// \class Lock On 2011/08/23 18:10:04, chandlerc wrote: > Stale comment. You can actually delete this line, Doxygen should pick it up by > looking below. Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:798: /// \class BuildLockset On 2011/08/23 18:10:04, chandlerc wrote: > Probably can remove this too Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:996: const Lockset LoopEntrySet, On 2011/08/23 18:10:04, chandlerc wrote: > indent is off Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1063: // Create a default initial lockset in case there are no predecessors. On 2011/08/23 18:10:04, chandlerc wrote: > This is no longer quite accurate... maybe just note that we already *have* a > default initial lockset. Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1099: for (CFGBlock::const_iterator BI = CurrBlock->begin(), BE = CurrBlock->end(); On 2011/08/23 18:10:04, chandlerc wrote: > 80 columns Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1143: On 2011/08/23 18:10:04, chandlerc wrote: > Can probably nix 2 of the 4 blank lines. Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:275: /// resolves to a lockable object. On 2011/08/23 18:10:04, chandlerc wrote: > Join with the previous line. Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:279: // Flag error if could not get record type for this argument. On 2011/08/23 18:10:04, chandlerc wrote: > no 2 spaces after '//' Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:299: int Sidx = 0, On 2011/08/23 18:10:04, chandlerc wrote: > Sidx? Huh? Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:301: for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) { On 2011/08/23 18:10:04, chandlerc wrote: > s/unsigned int/unsigned/ Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:307: QualType Arg_QT = ArgExp->getType(); On 2011/08/23 18:10:04, chandlerc wrote: > Arg_QT -> ArgTy Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:309: // Get record type. On 2011/08/23 18:10:04, chandlerc wrote: > This doesn't seem to to add information reall... Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:313: // Now check if we idx into a record type function param. On 2011/08/23 18:10:04, chandlerc wrote: > s/idx/index/ Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:320: uint64_t ParamIdx_from1 = ArgValue.getZExtValue(); On 2011/08/23 18:10:04, chandlerc wrote: > ParamIdxFromOne? Done. http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new... lib/Sema/SemaDeclAttr.cpp:321: uint64_t ParamIdx_from0 = ParamIdx_from1 - 1; On 2011/08/23 18:10:04, chandlerc wrote: > ParamIdxFromZero? Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
