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

[Haojian Wu via Phabricator via cfe-commits]

hokein abandoned this revision.
hokein added a comment.
Herald added a subscriber: dexonsmith.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85025/new/

https://reviews.llvm.org/D85025

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D85025#2201042 , @rsmith wrote:

> 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.




Comment at: clang/lib/AST/Expr.cpp:3757-3758
 case NPC_ValueDependentIsNull:
-  if (isTypeDependent() || getType()->isIntegralType(Ctx))
+  if ((!containsErrors() && isTypeDependent()) ||
+  getType()->isIntegralType(Ctx))
 return NPCK_ZeroExpression;

rsmith wrote:
> This change appears to be redundant: we handled all `containsErrors()` cases 
> before the `switch`.
oh, yeah, this is an oversight during the rebase.



Comment at: clang/lib/Sema/SemaExpr.cpp:14245-14247
+ExprResult Sema::CreateDependentBinOp(SourceLocation OpLoc,
+  BinaryOperatorKind Opc, Expr *LHS,
+  Expr *RHS) {

rsmith wrote:
> 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?
good point, thanks! I added some comments about this method, and make it 
private to make it less mis-unused.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85025/new/

https://reviews.llvm.org/D85025

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.




Comment at: clang/lib/AST/Expr.cpp:3757-3758
 case NPC_ValueDependentIsNull:
-  if (isTypeDependent() || getType()->isIntegralType(Ctx))
+  if ((!containsErrors() && isTypeDependent()) ||
+  getType()->isIntegralType(Ctx))
 return NPCK_ZeroExpression;

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



Comment at: clang/lib/Sema/SemaCast.cpp:2693
+  (DestType->isDependentType() || SrcExpr.get()->isTypeDependent() ||
+   SrcExpr.get()->isValueDependent())) {
+assert((DestType->containsErrors() || SrcExpr.get()->containsErrors() ||

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.)



Comment at: clang/lib/Sema/SemaExpr.cpp:14245-14247
+ExprResult Sema::CreateDependentBinOp(SourceLocation OpLoc,
+  BinaryOperatorKind Opc, Expr *LHS,
+  Expr *RHS) {

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?



Comment at: clang/test/Sema/error-dependence.c:10
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.





Comment at: clang/test/Sema/typo-correction.c:17
 int foobar;  // expected-note {{'foobar' declared here}}
-a = goobar ?: 4;  // expected-warning {{type specifier missing, defaults to 
'int'}} \
+new_a = goobar ?: 4;  // expected-warning {{type specifier missing, defaults 
to 'int'}} \
   // expected-error {{use of undeclared identifier 'goobar'; 
did you mean 'foobar'?}} \

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85025/new/

https://reviews.llvm.org/D85025

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
Herald added subscribers: cfe-commits, dang, arphaman, kbarton, nemanjai.
Herald added a project: clang.
hokein requested review of this revision.
Herald added a subscriber: wuzish.

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:

- build dependent binary operator for C: https://reviews.llvm.org/D84226
- build dependent callexpr in C for error-recovery: 
https://reviews.llvm.org/D84304
- suppress spurious "typecheck_cond_expect_scalar" diagnostic: 
https://reviews.llvm.org/D84322
- suppress spurious "err_typecheck_expect_scalar_operand" diagnostic : 
https://reviews.llvm.org/D84387
- adjust all existing tests when enabled recovery-expr for C: 
https://reviews.llvm.org/D84970

TESTED: ninja check-clang


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85025

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Parse/ParseExpr.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/CodeGen/builtins-ppc-error.c
  clang/test/CodeGen/builtins-systemz-zvector-error.c
  clang/test/CodeGen/builtins-systemz-zvector2-error.c
  clang/test/CodeGen/builtins-systemz-zvector3-error.c
  clang/test/Index/complete-switch.c
  clang/test/OpenMP/begin_declare_variant_messages.c
  clang/test/OpenMP/declare_variant_messages.c
  clang/test/Parser/objc-foreach-syntax.m
  clang/test/Sema/__try.c
  clang/test/Sema/enum.c
  clang/test/Sema/error-dependence.c
  clang/test/Sema/typo-correction.c

Index: clang/test/Sema/typo-correction.c
===
--- clang/test/Sema/typo-correction.c
+++ clang/test/Sema/typo-correction.c
@@ -14,7 +14,7 @@
   // expected-error {{use of undeclared identifier 'b'}}
 
 int foobar;  // expected-note {{'foobar' declared here}}
-a = goobar ?: 4;  // expected-warning {{type specifier missing, defaults to 'int'}} \
+new_a = goobar ?: 4;  // expected-warning {{type specifier missing, defaults to 'int'}} \
   // expected-error {{use of undeclared identifier 'goobar'; did you mean 'foobar'?}} \
   // expected-error {{initializer element is not a compile-time constant}}
 
@@ -50,10 +50,10 @@
   cabs(errij);  // expected-error {{use of undeclared identifier 'errij'}}
 }
 
-extern long afunction(int); // expected-note {{'afunction' declared here}}
+extern long afunction(int);
 void fn2() {
   f(THIS_IS_AN_ERROR, // expected-error {{use of undeclared identifier 'THIS_IS_AN_ERROR'}}
-afunction(afunction_));  // expected-error {{use of undeclared identifier 'afunction_'; did you mean 'afunction'?}}
+afunction(afunction_));  // expected-error {{use of undeclared identifier 'afunction_'}}
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
Index: clang/test/Sema/error-dependence.c
===
--- /dev/null
+++ clang/test/Sema/error-dependence.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type -fc-dependence %s
+
+int call(int); // expected-note 2{{'call' declared here}}
+
+void test1(int s) {
+  // verify "assigning to 'int' from incompatible type ''" is
+  // not emitted.
+  s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.
+  (*__builtin_classify_type)(1); // expected-error {{builtin functions must be directly called}}
+}
+
+void test2(int *ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or pointer
+  // type is required" is not emitted.
+  ptr > f ? ptr : f; // expected-error {{invalid operands to binary expression}}
+}
+
+void test3(float f) {
+  // verify diagnostic "operand of type '' where arithmetic or
+  // pointer type is required" is not emitted.
+  f = (float)call(); // expected-error {{too few arguments to function call}}
+}
Index: clang/test/Sema/enum.c
===
--- clang/test/Sema/enum.c
+++ clang/test/Sema/enum.c
@@ -100,7 +100,8 @@
 // PR7911
 extern enum PR7911T PR7911V; // expected-warning{{ISO C forbids forward references to 'enum' types}}
 void PR7911F() {
-  switch (PR7911V); // expected-error {{statement requires expression of integer type}}
+  switch (PR7911V); // expected-error {{statement requires expression of integer type}} \
+// expected-warning {{switch statement has empty