[PATCH] D49511: [Sema/Attribute] Check for noderef attribute

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked 2 inline comments as done.
leonardchan added inline comments.


================
Comment at: lib/Parse/ParseStmt.cpp:102-104
+  Actions.PushExpressionEvaluationContext(
+      Actions.ExprEvalContexts.back().Context);
   ParenBraceBracketBalancer BalancerRAIIObj(*this);
----------------
rsmith wrote:
> Hmm, we call `ParseStatementOrDeclaration` rather a lot. Can we pull this up into its ultimate callers instead:
> 
>   * `Parser::ParseFunctionDefinition`
>   * `Parser::ParseLateTemplatedFuncDef`
>   * `Parser::ParseLexedMethodDef`
>   * `Parser::ParseLexedObjCMethodDefs`
> 
> ? Actually, we can do better than that: those functions all call either `ActOnStartOfFunctionDef` or `ActOnStartOfObjCMethodDef` first, and `ActOnFinishFunctionBody` when they're done. We should put the `PushExpressionEvaluationContext` and `PopExpressionEvaluationContext` calls in those functions instead.
> 
> We're missing at least one other place: when parsing the initializer for a global variable, in C++ we call `Sema::ActOnCXXEnterDeclInitializer`, which pushes a potentially-evaluated context. But in C, the `InitializerScopeRAII` object (in `Parser::ParseDeclaratoinAfterDeclaratorAndAttributes`) doesn't call into `Sema` and we don't ever push a suitable expression evaluation context.
> 
> You can find if any other places are missing by removing `Sema`'s global `ExpressionEvaluationContext` and adding an assert that fires if we try to parse an expression with no `ExpressionEvaluationContext`, and then running the unit tests. (And we should check in that change to ensure this doesn't regress.)
@rsmith Would it be simpler to instead push and pop at the start and end of `Parser::ParseExternalDeclaration`? I'm currently working on your suggestion of removing the Sema global `ExpressionEvaluationContext` and find that a lot of accesses to the `ExprEvalContexts` stack stem from this method.

`ActOnStartOfFunctionDef` and `ActOnFinishFunctionBody` still work on anything in a function body, but I keep running into many cases for declarations declared globally that could be easily caught if instead I push and pop contexts at the start and end of `Parser::ParseExternalDeclaration`. Do you think this is a good idea or is there something that I may be missing from pushing and popping here?

This still accomplishes the goal of not reusing the global Sema context and I will still be able to check for `noderef` on every push and pop this way. 


Repository:
  rC Clang

https://reviews.llvm.org/D49511