[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:
http://llvm.org/docs/ProgrammersManual.html#isa
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,
tok::ObjCKeywordKind
ivarVisibility) {
assert(OIvar && "missing instance variable");
switch (ivarVisibility) {
case tok::objc_private:
OIvar->setAccessControl(ObjcIvarDecl::Private);
break;
case tok::objc_public:
OIvar->setAccessControl(ObjcIvarDecl::Public);
break;
case tok::objc_protected:
OIvar->setAccessControl(ObjcIvarDecl::Protected);
break;
case tok::objc_package:
OIvar->setAccessControl(ObjcIvarDecl::Package);
break;
default:
OIvar->setAccessControl(ObjcIvarDecl::None);
break;
}
}
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.
-Chris
More information about the cfe-commits
mailing list