[PATCH] D96944: [RecoveryAST] Add design doc to clang internal manual.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.


================
Comment at: clang/docs/InternalsManual.rst:1882
+  example.
+- representing invalid node: the invalid node is preserved in the AST in some
+  form, e.g. when the "declaration" part of the declaration contains semantic
----------------
hokein wrote:
> kadircet wrote:
> > rather than `some form` maybe say `while indicating the error` ?
> I'm not sure this will be better. `indicating error` seems to be a bit strong.
> 
> e.g. `if () {}`, this ill-formed statement, the AST node  is like below, which doesn't have an obvious error signal (we could argue the `OpaqueValueExpr` is )
> 
> ```
> `-IfStmt
>    |-OpaqueValueExpr 'bool'
>    `-CompoundStmt 
> ```
oh okay didn't know that. i thought we would always have a node which can indicate the error somehow (not having that is still surprising though).


================
Comment at: clang/docs/InternalsManual.rst:1971
+considered value-dependent, because its value isn't well-defined until the error
+is resolved. Among other things, this means that clang doesn't emit more errors
+where a RecoveryExpr is used as a constant (e.g. array size), but also won't try
----------------
sammccall wrote:
> hokein wrote:
> > kadircet wrote:
> > > IIRC, there were other problems with clang trying to emit secondary diags on such cases. It might be worth mentioning those too, again to warn future travellers about the side effects of making this non-value-dependent.
> > I think the existing `doesn't emit more errors` texts already indicate we suppress the secondary diags etc.
> Do you have an example? I chatted with Haojian offline about this and we couldn't find a good one apart from constant contexts. (And the wording of the standard suggests that value-dependence is a concept that's only interesting in constant contexts)
> Do you have an example?

unfortunately not :( but something tells me the particular bug was somehow about typo correction happening inside recoveryexprs ?

> I think the existing doesn't emit more errors texts already indicate we suppress the secondary diags etc.

yeah i guess you are right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96944