[cfe-commits] r41932 - in /cfe/trunk: Parse/ParseObjc.cpp Sema/Sema.h Sema/SemaDecl.cpp clang.xcodeproj/project.pbxproj include/clang/Parse/Action.h

Chris Lattner clattner at apple.com
Sun Sep 30 00:50:05 PDT 2007

On Sep 13, 2007, at 1:56 PM, Fariborz Jahanian wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=41932&view=rev
> Log:
> Patch for collecting ivars before running action on them.

Another minor nit-pick, this time an API one:

> +void Sema::ObjcAddInstanceVariable(DeclTy *ClassDecl, DeclTy **Ivar,
> +				   unsigned numIvars,
> +                                   tok::ObjCKeywordKind  
> *visibility) {
> +  assert((ClassDecl && numIvars) && "missing class or instance  
> variable");
>    ObjcInterfaceDecl *OInterface = dyn_cast<ObjcInterfaceDecl>(
>                                      static_cast<Decl *>(ClassDecl));
> +  assert (OInterface && "mistyped class");
> +  for (unsigned i = 0; i != numIvars; ++i) {
> +    ObjcIvarDecl *OIvar = dyn_cast<ObjcIvarDecl>(static_cast<Decl  
> *>(Ivar[i]));

In this case, you're using dyn_cast, but OIvar is never handled if it  
is null.  In actuality, you *know* that the type of the decl is  
guaranteed to be ObjcIvarDecl.  In situations like this, please use  
cast<type>(...) instead of dyn_cast<type>(...).

The difference between the two is that (in a release build with  
assertions disabled) the first turns into a standard C cast and the  
second turns into a dynamic type check.  In an assertion enabled  
build, the former does an assertion that the types match.

The end result of this is that dyn_cast or isa should be used if you  
aren't sure that the operand matches the type and that cast should be  
used when you are sure.  There are actually a couple more of these  
cast operators, documented here if you are interested:

The code above has actually been refactored a bit since this commit.   
Now just s/dyn_cast/cast on this line of SemaDecl.cpp:

ObjcSetIvarVisibility(dyn_cast<ObjcIvarDecl>(FD), visibility[i]);

> +    tok::ObjCKeywordKind ivarVisibility = visibility[i];
> +
> +    assert(OIvar && "mistyped instance variable");
> +
> +    switch (ivarVisibility) {
> +    case tok::objc_private:
> +      OIvar->setAccessControl(ObjcIvarDecl::Private);
> +      break;

On mainline you have:

static void ObjcSetIvarVisibility(ObjcIvarDecl *OIvar,
ivarVisibility) {
   assert(OIvar && "missing instance variable");
   switch (ivarVisibility) {
   case tok::objc_private:
   case tok::objc_public:
   case tok::objc_protected:
   case tok::objc_package:

As a minor matter of style, I'd suggest writing this as a pure  
translation function that doesn't mutate any state where possible.   
I'd suggest:

/// TranslateIvarVisibility - Translate visibility from a token ID to  
an AST enum value.
static ObjcIVar::AccessControl
TranslateIvarVisibility(tok::ObjCKeywordKind ivarVisibility) {
   switch (ivarVisibility) {
   case tok::objc_private: return ObjcIvarDecl::Private;
   case tok::objc_public: return ObjcIvarDecl::Public;
   case tok::objc_protected: return ObjcIvarDecl::Protected;
   case tok::objc_package: return ObjcIvarDecl::Package;
   default:  return ObjcIvarDecl::None;

And then move the OIvar->setAccessControl call into the caller of  
this method.  Also, is this the right thing for the default case?  It  
seems like there should be an assertion or something that the token  
kind coming in is the expected one.  Maybe not though, I don't know  
the code well enough to say for sure.

Also, please add comment free functions like this to document what  
they do.  The shorter or more trivial the function is, the shorter  
the comment can be of course.


More information about the cfe-commits mailing list