PATCH: private ivars

[Dmitri Gribenko]

On Tuesday, February 19, 2013, Adrian Prantl wrote:

>
> On Feb 18, 2013, at 4:28 PM, David Blaikie <dblaikie at gmail.com<javascript:;>>
> wrote:
> > On Mon, Feb 18, 2013 at 4:24 PM, Adrian Prantl <aprantl at apple.com<javascript:;>>
> wrote:
> > On Feb 15, 2013, at 3:59 PM, Dmitri Gribenko <gribozavr at gmail.com<javascript:;>>
> wrote:
> > >
> > > +    ObjCInterfaceDecl* decl = cast<ObjCInterfaceType>(Ty)->getDecl();
> > > +    if (decl)
> > >
> > > cast<> can not return null (it will either succeed or assert in
> > > +Asserts mode, or just produce a wrong value in -Asserts).  Use
> > > dyn_cast (that returns null on failure) or drop the check -- whatever
> > > is appropriate.  And there's a dyn_cast idiom:
> > >
> > > if (Foo *F = dyn_cast<Foo>(Blah))
> > > ... use F...
> >
> > I'm actually not checking the return value from cast<>() but the the
> result of getDecl() [which just happens to also return an
> ObjCInterfaceDecl]. Do you think I should use a temporary for the result of
> the cast to make it clearer?
> >
> > Then you're missing test cases, because dyn_cast doesn't propagate null.
> It should fail if you pass it null.
> >
> > You  probably want cast_or_null.
>
> At least in my particular case I'm not so sure about this. The code
> fragment we are talking about looks like this:
>
>  switch (Ty->getTypeClass()) {
>   ...
>   case Type::ObjCInterface: {
>     ObjCInterfaceDecl* Decl = cast<ObjCInterfaceType>(Ty)->getDecl();
>     if (Decl)
>        ...
>   }
>
> At this point Ty should always be nonzero and since we switch over the
> TypeClass the cast<> should always be safe, right?
>

My initial comment was wrong -- sorry.  But you can still improve the code
by moving the declaration into if:

if (ObjCInterfaceDecl* Decl = cast<ObjCInterfaceType>(Ty)->getDecl()) ...

Dmitri



-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130219/4cea9c6f/attachment.html>