[cfe-dev] Patch & feedback request

[Sebastian Redl]

Erik Verbruggen wrote:
> A new patch is attached
> On the map type: I checked http://llvm.org/docs/ProgrammersManual.html#ds_map and none of the other datatypes seem applicable.
>   
Yes, we really need SmallPtrMap. It would be perfect for this.

Some comments on the patch.

> -    if (!Handler->getExceptionDecl())
> +    if (!Handler->getExceptionDecl() && i < NumHandlers - 1)
>        return StmtError(Diag(Handler->getLocStart(),
> diag::err_early_catch_all));
> +
> +    // Detect handlers for the same type as an earlier one.
> +    const QualType QT = Handler->getCaughtType();
> +               if (QT.isNull())
> +                       continue;

You have some tabs in here, but that's a minor point.

If I remember correctly, Handler->getExceptionDecl() == 0 <=>
Handler->getCaughtType().isNull(). Thus, I think this could be better
written as

if (!Handler->getExceptionDecl()) {
  if (i < NumHandlers - 1)
    return StmtError(...);
  continue;
}

> +    const QualType CQT = QT.getTypePtr()->getCanonicalTypeInternal();
Why access the internal function directly? The usual way to get a
canonical type is
Context.getCanonicalType(QT);

Also, I don't like the name QT. It says exactly nothing. Consider
calling the variables Caught and CanonicalCaught (or overwrite Caught
with the canonical form).

> +    const QTypeToHandler::const_iterator it = HandlersForTypes.find(CQT);
Since you're inserting when you don't find anything, it's faster (just
one lookup) and IMO more readable to do this:

CXXCatchStmt *&PrevHandler = HandlersForTypes[CQT];
if (PrevHandler) {
  // Diags here
} else
  PrevHandler = Handler;

Sebastian