[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein updated this revision to Diff 251299. hokein marked 2 inline comments as done. hokein added a comment. address comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm.org/D65591 Files: clang/include/clang/AS

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

This revision was automatically updated to reflect the committed changes. Closed by commit rG4b0f1e12c243: [AST] Add a flag indicating if any subexpression had errors (authored by hokein). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ htt

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein added inline comments. Comment at: clang/lib/AST/ComputeDependence.cpp:174 ExprDependence clang::computeDependence(NoInitExpr *E) { return toExprDependence(E->getType()->getDependence()) & ExprDependence::Instantiation; sammccall wrote: > hok

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/ComputeDependence.cpp:174 ExprDependence clang::computeDependence(NoInitExpr *E) { return toExprDependence(E->getType()->getDependence

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein added inline comments. Comment at: clang/include/clang/AST/DependenceFlags.h:46 Instantiation = 2, +/// Placeholder, and is not used in actual Type. +Error = 4, sammccall wrote: > hokein wrote: > > sammccall wrote: > > > I'd like this comment

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein updated this revision to Diff 251077. hokein marked 10 inline comments as done. hokein added a comment. address comments, and add more missing-error-bit cases Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm.org/

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

sammccall added inline comments. Comment at: clang/include/clang/AST/DependenceFlags.h:46 Instantiation = 2, +/// Placeholder, and is not used in actual Type. +Error = 4, hokein wrote: > sammccall wrote: > > I'd like this comment to explain why it ex

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein added a comment. In D65591#1927076 , @sammccall wrote: > This is nice and simple, I do wonder whether we're missing cases in > ComputeDependence.cpp for the new bit. > > Can you take a pass through to check? (Assuming you haven't since picking up

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein updated this revision to Diff 251019. hokein marked 5 inline comments as done. hokein added a comment. - address comments - remove the placeholder errorbit in non-Expr dependence Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

sammccall added a comment. This is nice and simple, I do wonder whether we're missing cases in ComputeDependence.cpp for the new bit. Can you take a pass through to check? (Assuming you haven't since picking up this patch from Ilya). I'll do the same. Comment at: clang/inclu

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein updated this revision to Diff 250708. hokein added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm.org/D65591 Files: clang/include/clang/AST/ASTDumperUtils.h clang/include/clang/AST/Depend

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

hokein updated this revision to Diff 249039. hokein added a comment. - rebase to master - fix the crash Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm.org/D65591 Files: clang/include/clang/AST/ASTDumperUtils.h cl

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. As soon as I add a new enum value, this starts crashing some tests (even if we don't ever set the flag or serialize it). Not sure what causes it and won't have time to figure it out before I leave. Sorry about that :( Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov updated this revision to Diff 241706. ilya-biryukov added a comment. Herald added a subscriber: bmahjour. Rebase on top of refactored dependence propagation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. In D65591#1791876 , @rsmith wrote: > @ilya-biryukov Did I forget anything? SG, I don't think anything is missing. Thanks for the write-up! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1791876 , @rsmith wrote: > Summary of an off-line discussion with Ilya: I think that summary sounds very sensible. Thank you both for working on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

rsmith added a comment. Summary of an off-line discussion with Ilya: - The current situation where Clang can do delayed typo correction for C++ but not for C is not a good long-term position. The reason for that situation is that we use a dependent type to represent a type with errors. There ar

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. I would actually try to counter the type proposal and defend the `containsErrors` bit. Here's my thinking. Not knowing that `int()` had errors inside can lead to situations with bad diagnostics. We won't be able to suppress any non-type-related error, e.g. `int *a

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

rsmith added a comment. In D65591#1625744 , @aaron.ballman wrote: > In D65591#1625228 , @ilya-biryukov > wrote: > > > We can also assume they're cheap, use the visitor-based implementation and > > later switch if

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

merge_guards_bot added a comment. Build result: fail - 59344 tests passed, 2 failed and 812 were skipped. failed: LLVM.tools/llvm-ar/mri-utf8.test failed: LLVM.tools/llvm-objdump/X86/disassemble-functions.test Log files: cmake-log.txt

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov updated this revision to Diff 226106. ilya-biryukov added a comment. - Add the added bit to serialization - Mention contains-errors in the AST dump. Still not tests in this revision, see D69330 for an expression that is actually preserved and has t

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. I did some experiments with adding something similar to the `ErrorExpr` Aaron suggest. It has this flag set and does not get removed from the AST. I believe the best way to address Aaron's comments is to add tests mentioning this expression instead of trying to cat

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1626638 , @ilya-biryukov wrote: > In D65591#1625744 , @aaron.ballman > wrote: > > > The problem is: those bits are not infinite and we only have a handful left > > until b

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. In D65591#1625744 , @aaron.ballman wrote: > The problem is: those bits are not infinite and we only have a handful left > until bumping the allocation size; is this use case critical enough to use > one of those bits? I do

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1625228 , @ilya-biryukov wrote: > In D65591#1625183 , @aaron.ballman > wrote: > > > In D65591#1625154 , @riccibruno > > wrote: > >

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. In D65591#1625228 , @ilya-biryukov wrote: > In D65591#1625183 , @aaron.ballman > wrote: > > > In D65591#1625154 , @riccibruno > > wrote: > > >

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. In D65591#1625183 , @aaron.ballman wrote: > In D65591#1625154 , @riccibruno > wrote: > > > It seems that these two options are not exactly the same right ? The > > `ContainsError` b

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. In D65591#1625183 , @aaron.ballman wrote: > In D65591#1625154 , @riccibruno > wrote: > > > It seems that these two options are not exactly the same right ? The > > `ContainsError` bit

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1625154 , @riccibruno wrote: > It seems that these two options are not exactly the same right ? The > `ContainsError` bit is useful to quickly answer "Does this expression > contains an invalid sub-expression" wit

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. In D65591#1625154 , @riccibruno wrote: > It seems that these two options are not exactly the same right ? The > `ContainsError` bit is useful to quickly answer "Does this expression > contains an invalid sub-expression" wit

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

riccibruno added a comment. It seems that these two options are not exactly the same right ? The `ContainsError` bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an `ErrorExpr` node is useful to note that //this// s

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. Rather than adding a bit onto `Expr` to specify whether it's erroneous, have you considered making this a property of the type system by introducing an `ErrorExpr` AST node that other nodes can inherit from? I think that approach would work more naturally for thin

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. Gentle ping. @rsmith, please take a look when you have time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm.org/D65591 ___ cfe-commits mail

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov created this revision. ilya-biryukov added a reviewer: rsmith. Herald added a subscriber: jfb. Herald added a project: clang. ilya-biryukov added a child revision: D65592: [Parser] Avoid spurious 'missing template' error in presence of typos. The only subexpression that is considere