[cfe-commits] r111116 - in /cfe/trunk: include/clang/Checker/PathSensitive/ConstraintManager.h include/clang/Checker/PathSensitive/GRState.h lib/Checker/FlatStore.cpp lib/Checker/RegionStore.cpp lib/Checker/SimpleConstraintManager.cpp lib/Checker/SimpleConstraintManager.h lib/Checker/Store.cpp test/Analysis/outofbound.c

[Jordy Rose]

On Mon, 16 Aug 2010 09:06:54 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> On Aug 15, 2010, at 6:15 PM, Jordy Rose wrote:
> 
>> +
>> +  operator bool() const {
>> +    return getRegion() != NULL;
>> +  }
>> };
> 
> Hi Jordy,
> 
> I'm really mixed about this.  operator bool() is convenient, but in my
> experience it doesn't lead to better readability in the code.  Why not
just
> have clients call 'getRegion()', where the check becomes explicit? 
Using
> operator bool() also compounds the often abused use of bool.  For me,
bool
> is 'true' and 'false', not "is valid" or "is not valid".  If someone
> doesn't know the correct interpretation of operator bool() in a specific
> context, it is easy to mess things up.
> 
> Ted

I wasn't happy with calling getRegion() to be the way to check validity;
that's what it was before, but it's almost a detail of BindingKey that an
invalid key would have a null region. We really just want to know "is this
key valid" in the same way that we check "is this pointer valid"

But I'm not usually a fan of operator bool() either. I'll change it to
IsValid() when I get the chance this afternoon, then.

(Of course before this there were no null regions or invalid keys -- this
change pushed validation up a level.)