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

[Haojian Wu via Phabricator via cfe-commits]

hokein 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
----------------
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 
```


================
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?
> . can we have a couple of examples for the cases that we still drop the nodes today?

The concern of giving a dropped example is that it might be stale once it gets preserved and recovered in the future. So personally, I'd prefer to give a true example, it was the mismatched-argument function call.

I guess this is not too hard for readers to come up with an example, a pretty broken statement would be the case.


================
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?)
yeah, this is good point. added.




================
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
----------------
kadircet wrote:
> 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.
yeah, we already have this in the comment of `RecoveryExpr`.


================
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.
I think the existing `doesn't emit more errors` texts already indicate we suppress the secondary diags etc.


================
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”
----------------
kadircet wrote:
> might be worth mentioning this only exists for expressions.
in reality, they are complicated, these bits (template, contains-errors) are not only for expressions, but also for Type, NestedNameSpecifier, TemplateArgument.


================
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
----------------
kadircet wrote:
> not sure what second `this` is for
oh, removed it.


================
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
----------------
kadircet wrote:
> 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?
you're right, but the whole propagation process is complex, I think it needs more words to explain how and when it stops (and it is probably out-of-scope). I adjusted the word a bit to make less confusing.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96944