r175796 - Replace CFGElement llvm::cast support to be well-defined.

[Jordan Rose]

On Feb 21, 2013, at 22:53 , David Blaikie <dblaikie at gmail.com> wrote:

> [+Doug & Jordan who I discussed some of this with on IRC]
> 
> 
> On Thu, Feb 21, 2013 at 10:26 PM, Ted Kremenek <kremenek at apple.com> wrote:
> Hi David,
> 
> I've been mulling over this change, and while I think it is a great step in the right direction, one I thing that I'm a bit mixed about is code like this:
> 
>             if (CFGStmt CS = ElemIt->getAs<CFGStmt>()) {
> 
> The reason I don't like this is because one could easily just write:
> 
>   CFGStmt CS = ElemIt->getAs<CFGStmt>()
> 
> To play devil's advocate, the same is true of the llvm::cast usage, we write:
> 
> T* x = cast<T>(y);
> 
> All the time & have to know that cast never returns null (not even in the cast where you pass null (for that you need cast_or_null)) & thus go off and dereference 'x' unconditionally immediately after.
>  
> The problem is that, visually, there is no indication that this objct could be "invalid".  When we were dealing with pointer values, by common practice everyone knows that a pointer value is not valid if it is NULL.  When we use these objects directly we lose that intuition, and I fear people could easily do the "wrong thing" accidentally.
> 
> I agree with you.
> 
> Some context  here is that I started with TypeLoc & had exactly the same problem - I started with the "Optional<T> getAs()" version for TypeLoc & eventually discovered that TypeLoc itself was boolean testable for 'validity'. So I asked Doug on IRC if I should use TypeLoc's sense of validity or add the Optional on top of it still. To be honest, perhaps I didn't articulate the concern as well as you have here & so perhaps it wasn't apparent to him - but in any case, his preference was for the "T getAs()" form and to rely on TypeLoc's existing invalidity.
> 
> When it came to ProgramPoint and SVal there was no existing 'invalid' state that mapped well to this situation - I asked Jordan on IRC to see if I was reading these hierarchies correctly & he agreed (SVal has invalid/undef/unknown states but none match quite well enough to use in the API in this way) so I used Optional. But when I came across CFGEement I found a usable invalid state that looked much like TypeLoc, so, after chatting with Jordan again, used that - it seemed consistent with Doug's preference for the TypeLoc API as well.
>  
> While it may be more boilerplate, what do you think about getAs<> returning an Optional<CFGStmt> object instead?  We could template specialize Optional<CFGStmt> to not use more storage, etc., and basically do what you have here, except with something that really *looks* like it has a valid or invalid value respectively.  This would also match with the other changes you made to ProgramPoint.
> 
> What do you think?
> 
> Personally I'd prefer to do it the way you're suggesting.
> 
> My only concern is that there's already a sense of invalidity for CFGElement and TypeLocs that's publicly usable, so now the APIs would be returning Optional<T> where T is (without any type safety helping in this case) guaranteed to be "not invalid".
> 
> So here's a hypothetical step further: along with specializing Optional to use zero-cost "optionality" (I suppose that would negate the concern I have above - essentially Optional<T> would be another representation for "invalid T"), make it so that no code deals with raw invalid Ts (the only place where invalid Ts could be constructed would be in Optional, via friending). That way we get zero cost type safe invalid states.

My personal opinion on this is that we should actually remove the Invalid state from CFGStmt. It seems dangerous for exactly the reasons Ted has outlined.

I didn't push this on IRC because it would have meant more refactoring for David, and more actual work—not just a mechanical change but an actual restructuring of things. The mechanical change seemed simple enough that discussion wouldn't be necessary; evidently I was wrong about that. :-)

The only current use of the Invalid CFGElement() is in the BlockEntrance ProgramPoint's convenience accessor getFirstElement(), which is only used in two places. There's a second place that uses default-initialized CFGElements as placeholders for automatic destructors, but this doesn't actually care that the elements are Invalid beforehand.

For TypeLoc this is probably harder, but if we actually want a design that makes people do the right thing, this is the one I would want.

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130222/2d97903d/attachment.html>