[PATCH] D112287: [clang] Implement CFG construction for @try and @catch

[Hans Wennborg via Phabricator via cfe-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

lgtm, just some nits.



================
Comment at: clang/lib/Analysis/CFG.cpp:485
 
-  // This can point either to a try or a __try block. The frontend forbids
-  // mixing both kinds in one function, so having one for both is enough.
+  // This can point to either to a C++ try, an Objective-C @try, or a SEH __try.
+  // try and @try can be mixed and generally work the same.
----------------
One "to" too much in "to either to a ...".
Should probably be *an* SEH __try (at least in my head, that S is pronounced "ess").


================
Comment at: clang/lib/Analysis/CFG.cpp:3888
+
+  // We set Block to NULL to allow lazy creation of a new block (if necessary)
+  Block = nullptr;
----------------
ultra nit: period.


================
Comment at: clang/lib/Analysis/CFG.cpp:3895
 CFGBlock *CFGBuilder::VisitObjCAtThrowStmt(ObjCAtThrowStmt *S) {
   // FIXME: This isn't complete.  We basically treat @throw like a return
   //  statement.
----------------
does the fixme still stand?


================
Comment at: clang/lib/Analysis/CFG.cpp:3941
+  bool HasCatchAll = false;
+  for (unsigned h = 0; h < Terminator->getNumCatchStmts(); ++h) {
+    // The code after the try is the implicit successor.
----------------
Why h? Could this be a range for instead? Otherwise for (i = .., e = ..., i != e) is the classic llvm style.


================
Comment at: clang/lib/Analysis/CFG.cpp:5790
+      OS << "@catch (";
+      if (CS->getCatchParamDecl())
+        CS->getCatchParamDecl()->print(OS, PrintingPolicy(Helper.getLangOpts()),
----------------
maybe

```
if (const VarDecl *PD = CS->getCatchParamDecl())
  PD->print(...)
```


================
Comment at: clang/test/Sema/warn-unreachable.m:1
+// RUN: %clang_cc1 %s -fsyntax-only -fobjc-exceptions -verify -Wunreachable-code
+
----------------
Not important, but I think it's much more common to have %s at the end. Same for the other test file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112287/new/

https://reviews.llvm.org/D112287