rnk added a comment.

Thanks!

John touched this code last in https://reviews.llvm.org/rL112038 in 2010, so 
maybe he has some thoughts on how to clean this and the follow-up. I think I'll 
land this as is since it fixes the crash and we can discuss more improvements 
in https://reviews.llvm.org/D44039.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:12412
+    // anyway so we can try to parse the function body.
+    PushFunctionScope();
     return D;
----------------
thakis wrote:
> Feels a bit long-term risky since ActOnStartOfFunctionDef() and 
> ActOnFinishFunctionBody() both need to know about this special-case 
> invariant. Maybe it's worth to add a FakeFunctionScopeCount member to sema in 
> +assert builds, and to increment that here, assert it's > 0 in the other 
> place and decrement it there, and then assert it's 0 at end of TU?
Well, it's more like these early returns break the invariant that 
`ActOnStartOfFunctionDef` pushes a function scope. We could try to clean it up 
with an RAII helper or an layer of function call that ensures that we always 
push and pop on start and finish, but I'll leave that for the follow-up.


================
Comment at: clang/test/SemaCXX/pr36536.cpp:19
+  // this when they forget to close a namespace, and we'd generate far fewer
+  // errors if names in Foo were in scope.
+  // expected-error@+1 {{unknown type name 'NameInClass'}}
----------------
thakis wrote:
> Not 100% clear to me what the FIXME is here. Maybe "FIXME: We should improve 
> our recovery to redeclare...." if that's what's meant.
I rewrote this to clarify things.


Repository:
  rC Clang

https://reviews.llvm.org/D43980



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to