This is an archive of the discontinued LLVM Phabricator instance.

[RecoveryExpr]WIP: Support dependence in C-only codepath
AbandonedPublic

Authored by hokein on Jul 31 2020, 5:55 AM.

Details

Reviewers
rsmith
Summary

This is a large patch containing all required changes, want early
feedback.

what does this patch do?

  • support dependent mechanisam (only for error-recovery) in C-only codepath;
  • allows building RecoveryExpr for C;
  • remove all early TypoCorrection technical debet in clang;

This will be split into different patches:

TESTED: ninja check-clang

Diff Detail

Event Timeline

hokein created this revision.Jul 31 2020, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2020, 5:55 AM
hokein requested review of this revision.Jul 31 2020, 5:55 AM
hokein added a subscriber: sammccall.
rsmith added a comment.Aug 6 2020, 1:42 PM

Looks reasonable to me. I expect you'll find more places that need to learn how to handle dependent types in C, but this looks like a solid start.

clang/lib/AST/Expr.cpp
3757–3758

This change appears to be redundant: we handled all containsErrors() cases before the switch.

clang/lib/Sema/SemaCast.cpp
2693

Do we care about value-dependence here? Very little of C cast semantics depends on the evaluated value of the expression -- I think this only matters for null pointer constants. If we care about the value-dependent cast to pointer case, maybe we should special-case that?

(It looks like the special-case handling for OpenCL event_t will also need a value-dependence check.)

clang/lib/Sema/SemaExpr.cpp
14245–14247

This function seems dangerous: in C++, we need to perform unqualified lookups from the template definition context when creating a dependent binary operator, and it's only correct to use this if such lookup found nothing.

Perhaps you could add something to the name of the function to indicate that it should only be used when there are no unqualified lookup results?

clang/test/Sema/error-dependence.c
10
clang/test/Sema/typo-correction.c
17

I assume we're getting a redefinition error for a now. If so, can you test that uses of a after line 13 don't produce errors?

Looks reasonable to me. I expect you'll find more places that need to learn how to handle dependent types in C, but this looks like a solid start.

Thanks! Yeah, we'd need to a detailed plan to roll this out (similar to the recovery-ast in C++) to catch possibly-missing cases.

I'll address your comments in split patches. I think the next plan is be to move forward the code reviews, Sam agreed to review those and I'll add you in the Subscribers.

clang/lib/AST/Expr.cpp
3757–3758

oh, yeah, this is an oversight during the rebase.

clang/lib/Sema/SemaExpr.cpp
14245–14247

good point, thanks! I added some comments about this method, and make it private to make it less mis-unused.

hokein abandoned this revision.Jan 18 2021, 5:13 AM

Thanks for @rsmith's initial comments.

This is done, all related patches have been landed in upstream, and this feature is enabled by default for all languages.