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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
