[PATCH] D54047: Check TUScope is valid before use

[Jan Korous via Phabricator via cfe-commits]

jkorous added reviewers: rjmccall, doug.gregor.
jkorous added subscribers: rjmccall, doug.gregor.
jkorous added a comment.

Adding @rjmccall and @doug.gregor who might have some insight.

Honestly, I don't know how IWYU works and my familiarity with Sema is limited so bear with me.

>From what I see `TUScope` is set only in one place in clang and that is `void Sema::ActOnTranslationUnitScope(Scope *S)`.

  > grep -nr --exclude="*/test/*" "TUScope\s\+=" .
  ./lib/Sema/Sema.cpp:70:  TUScope = S;
  ./lib/Sema/Sema.cpp:149:  TUScope = nullptr;
  ./lib/Sema/Sema.cpp:946:      TUScope = nullptr;
  ./lib/Sema/Sema.cpp:1168:    TUScope = nullptr;

(TUScope seems not to be copied anywhere.)

  > grep -nr --exclude="*/test/*" "=\s\+TUScope" .

`ActOnTranslationUnitScope` itself is called only from `void Parser::Initialize()`.

Just an idea - can't your code mimic behavior of `Parser::Initialize()`?

Other than that it seems that:

1. The code obviously assumes `TUScope != nullptr`.
2. We aren't aware of hitting this case in clang.

So even with my limited insight I would be inclined to make the assumption explicit. The whole block of code after `if (isValidVarArgType(E->getType()) == VAK_Undefined)` seems to be mostly checks for errors so `ExprError()` seems like the right error reporting mechanism here as it's part of this method's interface and randomly picked caller's code in `SemaOverload.cpp` seems to handle such case.

Re: tests Since you are using Sema from your code I assume you should be able to create a regression test (with a suitable code input) as a minimal reproducer. You might take a look how is `Sema` set up in unittests.


Repository:
  rC Clang

https://reviews.llvm.org/D54047