[PATCH] D83213: [AST][RecoveryExpr] Don't set the instantiation-bit.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I think this depends on how we interpret the instantiation-dependent bit. In clang, it currently has two semantics:

1. an expression (somehow) depends on a template parameter;
2. an expression is dependent;

Prior to RecoveryExpression being added, 1 and 2 are equal, so we use `isInstantiationDependent` to check whether the code is dependent. This is fine.

However since we now generalize the "dependent" concept to mean "depends on a template parameter, or an error", checking `isInstantiationDependent()` is incomplete to determine the code is dependent.

what we could do?

**Option 1** (what this patch does):

instantiation-dependence still  implies the expression involves a template parameter -- if a recovery expression doesn't involves a template parameter, we don't set this flag

pros:

- avoid unnecessary instantiations if an AST node doesn't depend on template parameters but contain errorss;
- more precise semantics (maybe for better error-tracking, diagnostics), we can express a recovery-expr involves a template parameter (`contains-errors` + `instantiation-dep`), or not (`contains-erros` + `no instantiation-dep`)

cons:

- as you mentioned above, we violate the "nothing can be dependent if it is not instantiation dependent" assumption, which may leads us to fix a long tail of bugs (high effort), and the fix is basically adding `containsErrors` to where `isInstantiaionDependent` is used.

This makes me feel like we're going back to "use containsErrors everywhere" solution. Not sure what pros would buy us, maybe it is not worth the effort.

**Option 2**:

Keep it as it-is now, always set it, then the `instantiation-dependent` would mean "an expression depends on a template parameter or an error".

pros:

- everything would work without changing anything!

cons:

- keep using the instantiation name is confusing; we could do clarify the document of isInstantiation

While writing this, I'm starting to like the option2, any other thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83213