ojhunt wrote: > I think my original implementation fixed the original issue but introduced a > new bug (your example). I assumed `contains errors` always comes with > `instantiation-dependent`, which is true for a `RecoveryExpr` but not for a > reference to a variable declaration.
No, it "fixed" the assertion by removing the path that caused the assertion to fail rather than correcting the diagnostic. It was wrong. An assertion failure is _not_ a bug. An assertion failure is indicative of something else going wrong. In this case the error was not handling all the possible error enumerations. > I reproduced it. Indicates a misunderstanding of what is expected from review prior to posting a PR - reproducing a failure, and then having the error go away is not fixing the failure. > `containsErrors()` is also set on a reference to a variable with a bad > initializer, so the early return rejected valid statements. I dropped it, > kept `NotAValidExpression`, and added the missing option to the note. Your > example is in the tests. I would like you to try and come up with additional tests yourself - you need to think about how your code might go wrong, or cases _around_ the current failure. You want to do this because if you're too focused on just the specific failure case you first encounter it is extremely easy to fall into piecemeal or ad-hoc fixes rather than systemic corrections. https://github.com/llvm/llvm-project/pull/228914 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
