[PATCH] D132918: [clang] Fix a crash in constant evaluation

Wed Sep 7 02:04:38 PDT 2022

ilya-biryukov accepted this revision as: ilya-biryukov.
ilya-biryukov added a comment.

In D132918#3773237 <https://reviews.llvm.org/D132918#3773237>, @shafik wrote:

> This is helpful information but I am not sure this convinces me that `EvaluateVarDecl` is the correct place to check. Why not in `EvaluateDecl` or `EvaluateStmt`? Both locations we could also check.

So the options are:

1. `assert(Decl->isValid())` in `EvaluateVarDecl` and checks at call sites in `EvaluateDecl` and `EvaluateStmt`.
2. a single check in `EvaluateVarDecl`.

The actual observable behavior of the compiler seems equivalent (`EvaluateDecl` can only fail inside `EvaluateVarDecl` anyway).
I think either is fine, neither of the approaches seems to be a big win over the other.

I will stamp this as I'm already in the reviewers list, think this change is ok and believe that Aaron's comments were addressed.
However, I still suggest to give time for @shafik to respond before landing this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132918