Page MenuHomePhabricator

[AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery.
ClosedPublic

Authored by hokein on Jul 21 2020, 2:41 AM.

Details

Summary

Introduce a new cc1 flag to enable build the dependent AST nodes for C.

see the whole view in: https://reviews.llvm.org/D85025

Diff Detail

Event Timeline

hokein created this revision.Jul 21 2020, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 2:41 AM
hokein marked an inline comment as done.Jul 21 2020, 2:44 AM
hokein added inline comments.
clang/lib/Sema/SemaExpr.cpp
14271

I think we probably need a new cc1 flag e.g. CDependenceType to control the typo-correction and whether we build dependent ast node for C.

hokein updated this revision to Diff 279751.Jul 22 2020, 3:16 AM

Add a cc1 flag to enable building dependent nodes in C.

hokein updated this revision to Diff 284358.Aug 10 2020, 7:19 AM
  • rebase;
  • add comments for CreateDependentBinOp, and make it private based on Richard's comment;
hokein edited the summary of this revision. (Show Details)Aug 10 2020, 7:20 AM
hokein retitled this revision from [AST][RecoveryExpr] Support dependent binary operator in C for error recovery. to [AST][RecoveryExpr] Support dependent binary operator in C for error recovery Part 1..Aug 17 2020, 4:02 AM
hokein retitled this revision from [AST][RecoveryExpr] Support dependent binary operator in C for error recovery Part 1. to [AST][RecoveryExpr] Part1: Support dependent binary operator in C for error recovery..Aug 17 2020, 4:05 AM
sammccall added inline comments.Sep 18 2020, 3:21 AM
clang/include/clang/Basic/LangOptions.def
153 ↗(On Diff #284358)

Why is this not just the RecoveryAST flag? Is there some use of dependence outside the error-handling path?

If the language is C, recovery-AST only works if this flag is on too. So they might as well always have the same value.
If the language is C++, this flag is meaningless, right?

sammccall added inline comments.Sep 18 2020, 3:50 AM
clang/lib/Sema/SemaExpr.cpp
14271

I think we can do better on types here: for several binary operators the type is boolean in C regardless of operands. (except for vector types I think, but those might be rare enough to ignore in recovery)

14274

In C the result type is the unqualified type of the LHS

14276

I don't think we need to override the computation types, we can just leave them null?

hokein updated this revision to Diff 294901.Sep 29 2020, 12:45 AM
hokein marked 2 inline comments as done.

address comments:

  • reuse the RecoveryAST to control the early typo-correction;
  • uses the fixed result type for some operators
hokein added inline comments.Sep 29 2020, 12:47 AM
clang/include/clang/Basic/LangOptions.def
153 ↗(On Diff #284358)

This flag is mainly for turning off early typo-corrections for C.

Thinking more about this, you're right, looks like using the RecoveryAST is possible:

  • these early typo-corrections applies non-C++ code, and have to be there until we fully implemented recovery-expr-on-c;
  • the current default mode of RecoveryAST: on for C++, and off for non-C++;
  • for recovery-expr-on-c development, we need to turn on the RecoveryAST and turn off these early typo-corrections;

so if (!S.getLangOpts().CPlusPlus && !S.getLangOpts().RecoveryAST) { works.

sammccall added inline comments.Sep 29 2020, 12:55 AM
clang/include/clang/Basic/LangOptions.def
153 ↗(On Diff #284358)

If that's going to be a common condition, I'd suggest a helper function like

/// (explanation referencing early typo correction here)
isDependenceAllowed(const LangOpts &LO) { return CPlusPlus || RecoveryAST; }

and then guard early typo correction with if (!isDependenceAllowed())

clang/include/clang/Sema/Sema.h
5169 ↗(On Diff #284358)

nit: unqualified
nit: remove "results"

hokein updated this revision to Diff 294903.Sep 29 2020, 1:18 AM
hokein marked an inline comment as done.

add isDependenceAllowed function.

clang/include/clang/Basic/LangOptions.def
153 ↗(On Diff #284358)

sounds a good idea.

clang/include/clang/Sema/Sema.h
5169 ↗(On Diff #284358)

this was in the first snapshot, we don't need this change now.

hokein added a comment.Mon, Oct 5, 3:36 AM

I think this patch is ready for another round of review.

sammccall accepted this revision.Mon, Oct 5, 6:09 AM
sammccall added inline comments.
clang/include/clang/AST/ASTContext.h
668

This is a bit hard to follow.
I think it's two separate sentences, and the second is something like
"If this condition is false, typo correction must be performed eagerly rather than delayed in many places, as it makes use of dependent types".

clang/lib/Sema/SemaExpr.cpp
14373

isAssignmentOp instead? including = itself

14391

comma could get the RHS :-)
May get some mileage for comma operators in macros, not sure though.

(Other aspect is value category: fortunately *in C* comma is an rvalue)

clang/test/AST/ast-dump-recovery.c
45–48

'int *' lvalue contains-errors '='

52

'int' lvalue contains-errors '+='

This revision is now accepted and ready to land.Mon, Oct 5, 6:09 AM
hokein updated this revision to Diff 296218.Mon, Oct 5, 10:12 AM
hokein marked 2 inline comments as done.

address comments.

clang/lib/Sema/SemaExpr.cpp
14373

Simple assignment = doesn't belong to CompoundAssignOperator, it should be BinaryOperator.

Added the handling logic in the switch-case below.

clang/test/AST/ast-dump-recovery.c
45–48

oh, this spots a bug in our code -- unlike C++, it should not be a lvalue.

From C [6.15.16] p3:

An assignment expression has the value of the left operand after the assignment, but is not an lvalue.

sammccall accepted this revision.Mon, Oct 5, 11:50 AM
sammccall added inline comments.
clang/lib/Sema/SemaExpr.cpp
14381

now the only thing that differs between the cases is the type. You could consider putting the type in a variable, and factoring the Create after the switch.

14393

(why the line break here)

hokein updated this revision to Diff 296367.Mon, Oct 5, 11:52 PM
hokein marked 2 inline comments as done.

address comments.

This revision was landed with ongoing or failed builds.Mon, Oct 5, 11:57 PM
This revision was automatically updated to reflect the committed changes.