[cfe-commits] r71467 - in /cfe/trunk: lib/CodeGen/CGObjCMac.cpp test/CodeGenObjC/metadata-symbols-64.m

[Daniel Dunbar]

Nice! A few comments:

On Mon, May 11, 2009 at 12:25 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:
> +  /// NoneLegacyDispatchMethods - List of methods for which we do *not* generate
> +  /// legacy messaging dispatch.
> +  llvm::StringMap<bool> NoneLegacyDispatchMethods;

None -> Non? Also, this would be better to be a DenseSet<Selector>, we
don't want to convert selectors to strings all the time.

> +
> +  /// LegacyDispatchedSelector - Returns true if SEL is not in the list of
> +  /// NoneLegacyDispatchMethods; flase otherwise.

Misspelled false...

> +  // In 64bit ABI, type must be assumed VARARG. It 32bit abi,

it -> in

>   } else if (ResultType->isFloatingType()) {
>     // FIXME: Sadly, this is wrong. This actually depends on the
>     // architecture. This happens to be right for x86-32 though.

Is this FIXME fixed? Seems dangerous if not...

> +/// LegacyDispatchedSelector - Returns true if SEL is not in the list of
> +/// NoneLegacyDispatchMethods; flase otherwise. What this means is that

flase -> false

> +/// except for the 19 selectors in the list, we generate 32bit-style
> +/// message dispatch call for all the rest.
> +///
> +bool CGObjCNonFragileABIMac::LegacyDispatchedSelector(Selector Sel) {
> +  // FIXME! Lagcy API in Nonfragile ABI is for 10.6 on

Lagcy -> Legacy

> +  if (NoneLegacyDispatchMethods.empty()) {
> +    NoneLegacyDispatchMethods["allocWithZone:"] = true;
> +    NoneLegacyDispatchMethods["alloc"] = true;
> +    NoneLegacyDispatchMethods["class"] = true;
> +    NoneLegacyDispatchMethods["self"] = true;
> +    NoneLegacyDispatchMethods["isKindOfClass:"] = true;
> +    NoneLegacyDispatchMethods["respondsToSelector:"] = true;
> +    NoneLegacyDispatchMethods["isFlipped"] = true;
> +    NoneLegacyDispatchMethods["length"] = true;
> +    NoneLegacyDispatchMethods["objectForKey:"] = true;
> +    NoneLegacyDispatchMethods["count"] = true;
> +    NoneLegacyDispatchMethods["objectAtIndex:"] = true;
> +    NoneLegacyDispatchMethods["isEqualToString:"] = true;
> +    NoneLegacyDispatchMethods["isEqual:"] = true;
> +    NoneLegacyDispatchMethods["retain"] = true;
> +    NoneLegacyDispatchMethods["release"] = true;
> +    NoneLegacyDispatchMethods["autorelease"] = true;
> +    NoneLegacyDispatchMethods["hash"] = true;
> +    NoneLegacyDispatchMethods["addObject:"] = true;
> +    NoneLegacyDispatchMethods["countByEnumeratingWithState:objects:count:"]
> +      = true;
> +  }
> +  const char *name = Sel.getAsString().c_str();
> +  if (NoneLegacyDispatchMethods[name])
> +    return false;
> +  return true;

The use of name here isn't safe, Sel.getAsString() returns a temporary
string which will be destroyed. However, this problem goes away of the
table changes to be mapped by Selector instead of string.

Thanks,
 - Daniel