r175594 - Replace SVal llvm::cast support to be well-defined.

[David Blaikie]

On Wed, Feb 20, 2013 at 10:50 AM, Jordan Rose <jordan_rose at apple.com> 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 <dblaikie at gmail.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130220/bda07aa1/attachment.html>