On Wed, Feb 20, 2013 at 10:50 AM, Jordan Rose <[email protected]> wrote:
> Thanks, David! > > Is it all right if I add Optional to clang/Basic/LLVM.h, so that it > doesn't have to appear everywhere? > Oh, sure - can't see any reason why not. I realized it's already 'using' in a few of the files I touched & I meant to go back & remove unnecessary qualifications before committing. Did this in r175679 after removing an old & crufty version of Optional found in lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp in r175678 > > A few comments inline. > > > On Feb 19, 2013, at 21:52 , David Blaikie <[email protected]> wrote: > > > Author: dblaikie > > Date: Tue Feb 19 23:52:05 2013 > > New Revision: 175594 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=175594&view=rev > > Log: > > Replace SVal llvm::cast support to be well-defined. > > > > See r175462 for another example/more details. > > > > Modified: > > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h > > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h > > > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h > > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h > > cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/StreamChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/ProgramState.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/SValBuilder.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/SVals.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp > > cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp > > > > > > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h > > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h?rev=175594&r1=175593&r2=175594&view=diff > > > ============================================================================== > > --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h > (original) > > +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h > Tue Feb 19 23:52:05 2013 > > @@ -69,6 +69,25 @@ protected: > > public: > > explicit SVal() : Data(0), Kind(0) {} > > > > + template<typename T> > > + T castAs() const { > > + assert(T::isType(*this)); > > + T t; > > + SVal& sv = t; > > + sv = *this; > > + return t; > > + } > > + > > + template<typename T> > > + llvm::Optional<T> getAs() const { > > + if (!T::isType(*this)) > > + return llvm::Optional<T>(); > > + T t; > > + SVal& sv = t; > > + sv = *this; > > + return t; > > + } > > Bikeshedding to isKind, instead of isType, since "type" is such an > overloaded word in Clang already. > Sounds OK to me - just wanted to make sure it wasn't "classof" to ensure it would not be accidentally compatible with llvm::cast (even being private/friended to base could mean llvm::casts could be used in the implementation of SVals which has a fair few casts that could easily have been missed). Renamed in r175676 > Also, capital variable names, and ampersand on the variable name rather > than the type. > > I guess Val->SVal::operator=(*this) isn't really cleaner than the > reference trick, huh. Yeah - if we had an implicit_cast template I'd consider using that: implicit_cast<SVal&>(t) = *this; But any/all of those options are welcome, none seem terribly much better than others. > A bunch of these types didn't have default constructors, though, so I'm > wondering if adding T(const SVal &) constructors makes more sense. Eh. > It was easier to add default constructors than forwarding constructors - but, yeah, not by much. I'm open to the alternative (that's actually what I'd originally considered, before Richard Smith thought up the base op= method (which, admittedly, worked better in the TypeLoc hierarchy where types were all already default constructible) > > - bool isExpression() { > > + bool isExpression() const { > > return !isa<SymbolData>(getSymbol()); > > } > > Heh, thanks. > Turns out I didn't actually /need/ this once I fixed Optional's const correctness issues (didn't have non-const overloads for op*/op->) but seemed appropriate anyway. > > if (originalRegion != R) { > > if (Optional<SVal> OV = B.getDefaultBinding(R)) { > > - if (const nonloc::LazyCompoundVal *V = > > - dyn_cast<nonloc::LazyCompoundVal>(OV.getPointer())) > > + if (llvm::Optional<nonloc::LazyCompoundVal> V = > > + OV.getPointer()->getAs<nonloc::LazyCompoundVal>()) > > This is going through another Optional, so you should be able to just use > OV->getAs now. > Oh, handy - didn't notice. (I hesitate to suggest it: should we make llvm::cast machinery able to treat Optional like a pointer? (I wonder if we already do this for OwningPtr & whatever other smart pointers we have in LLVM)) The transformation would've been a bit more natural/obvious if this code had used op* in the first place. > > @@ -1669,9 +1670,8 @@ NonLoc RegionStoreManager::createLazyBin > > // If we already have a lazy binding, and it's for the whole structure, > > // don't create a new lazy binding. > > if (Optional<SVal> V = B.getDefaultBinding(R)) { > > - const nonloc::LazyCompoundVal *LCV = > > - dyn_cast<nonloc::LazyCompoundVal>(V.getPointer()); > > - if (LCV) { > > + if (llvm::Optional<nonloc::LazyCompoundVal> LCV = > > + V.getPointer()->getAs<nonloc::LazyCompoundVal>()) { > > Ditto. > Fixed both of these in r175677. Thanks, - David
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
