Page MenuHomePhabricator

[AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery.
ClosedPublic

Authored by hokein on Jul 22 2020, 3:57 AM.

Diff Detail

Event Timeline

hokein created this revision.Jul 22 2020, 3:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2020, 3:57 AM
hokein retitled this revision from [AST][RecoveryExpr] Build dependent callexpr in C for error-recovery. to [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery..Aug 17 2020, 4:02 AM
sammccall added inline comments.Sep 18 2020, 3:35 AM
clang/lib/Sema/SemaExpr.cpp
6378

why is CDependence a condition and not an assertion inside the if?
(If it's for performance, then we're going to lose this performance when flipping the flag, so I'd consider just flipping it now)

If you're using it as a short-hand for "is this plain C and also has recovery enabled" then I'd avoid that - it's not clear that this is a language check.

6385

Is this really the right place vs in BuildResolvedCallExpr?

6385

why DependentTy? shouldn't it be the return type of the function, if available?

clang/test/AST/ast-dump-recovery.c
102

i'm not sure what the int (int) is about - can you remove it from the assertion?

hokein updated this revision to Diff 296633.Wed, Oct 7, 3:20 AM
hokein marked 4 inline comments as done.

address comments.

hokein updated this revision to Diff 296634.Wed, Oct 7, 3:21 AM

fix format.

clang/lib/Sema/SemaExpr.cpp
6385

Is this really the right place vs in BuildResolvedCallExpr?

BuildResolvedCallExpr does some semantic analysis/check, it might emit diagnostics, which we want to avoid. And putting the logic here also aligns with what C++ code path does.

why DependentTy? shouldn't it be the return type of the function, if available?

oh, yeah, I missed this. Added.

sammccall accepted this revision.Fri, Oct 9, 4:09 AM
This revision is now accepted and ready to land.Fri, Oct 9, 4:09 AM
This revision was landed with ongoing or failed builds.Mon, Oct 12, 2:18 AM
This revision was automatically updated to reflect the committed changes.