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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks! i think this mostly looks great, leaving a couple of suggestions and questions in 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
----------------
rather than `some form` maybe say `while indicating the error` ?


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


================
Comment at: clang/docs/InternalsManual.rst:1889
+
+With these strategies, clang surfaces nice diagnostics, and provides a rich AST
+reflecting the written source code as much as possible even for broken code to
----------------
s/nice/better


================
Comment at: clang/docs/InternalsManual.rst:1890
+With these strategies, clang surfaces nice diagnostics, and provides a rich AST
+reflecting the written source code as much as possible even for broken code to
+AST consumers.
----------------
maybe drop `to AST consumers` or move it to the begining of the statement i.e.
`provides AST consumers with a rich AST reflecting the written source code as ....`


================
Comment at: clang/docs/InternalsManual.rst:1910
+Without Recovery AST, the invalid function call expression (and its child
+expressions) was dropped in the AST:
+
----------------
s/was/would be


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


================
Comment at: clang/docs/InternalsManual.rst:1959
+
+In cases where we are confident about the concrete type (e.g. the return type
+for a broken non-overloaded function call), the ``RecoveryExpr`` will have this
----------------
that's great to know! i would expect it to be there already, but i think we should have this as a comment within the code too, so that future maintainers can feel confident when setting the type of a recovery expr.


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


================
Comment at: clang/docs/InternalsManual.rst:1978
+
+Beyond the template dependence bits, we add a new “ContainsErrors” bit to
+express “Does this expression or this or anything within it contain errors”
----------------
might be worth mentioning this only exists for expressions.


================
Comment at: clang/docs/InternalsManual.rst:1979
+Beyond the template dependence bits, we add a new “ContainsErrors” bit to
+express “Does this expression or this or anything within it contain errors”
+semantic, this bit is always set for RecoveryExpr, and propagated to parent
----------------
not sure what second `this` is for


================
Comment at: clang/docs/InternalsManual.rst:1980
+express “Does this expression or this or anything within it contain errors”
+semantic, this bit is always set for RecoveryExpr, and propagated to parent
+nodes, this provides a fast way to query whether any (recursive) child of an
----------------
this sounds like it is propagated all the way to the TUDecl, but i suppose that's not the case. can you elaborate on when the propagation stops?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96944