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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
