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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added reviewers: rsmith, kadircet.
sammccall added a comment.
This revision is now accepted and ready to land.

I've reviewed this previously and am happy with how it ended up.
But Kadir's comments are good, and we should also wait a few days in case others want changes.



================
Comment at: clang/docs/InternalsManual.rst:1886
+- dropping invalid node: this often happens for errors that we don’t have
+  graceful recovery, prior to Recovery AST, a mismatched-argument function call
+  expression was dropped though a CallExpr was created for semantic analysis.
----------------
kadircet wrote:
> so far we've always been talking about the current state right? comparison to past seems surprising now. can we have a couple of examples for the cases that we still drop the nodes today?
nit: "graceful recovery. Prior to..."


================
Comment at: clang/docs/InternalsManual.rst:1933
+
+An alternative is to use existing Exprs, e.g. CallExpr for the above example.
+The basic trade off is that jamming the data we have into CallExpr forces us to
----------------
kadircet wrote:
> this talks about why it would be hard to make use of `CallExpr`s but doesn't say what we would gain by doing so. I suppose it would come with the benefit of preserving more details about the source code, like locations for parantheses and knowing the type of the expr? (or are these still accessible e.g. do we have an enum in RecoveryExpr telling us about its type?)
Ooh, you're right: this says "the trade off" but only mentions one side :-)

Maybe "This would capture more call details and allow it to be treated uniformly with valid CallExprs. However, jamming..."


================
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
----------------
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.
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96944