[PATCH] D89046: [AST] Build recovery expression by default for all language.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang/test/CodeGen/builtins-ppc-error.c:51
 void testCTF(int index) {
-  vec_ctf(vsi, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}}
-  vec_ctf(vui, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}}
+  vec_ctf(vsi, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}} expected-error {{argument to '__builtin_altivec_vcfux' must be a constant integer}}
+  vec_ctf(vui, index); //expected-error {{argument to '__builtin_altivec_vcfsx' must be a constant integer}} expected-error {{argument to '__builtin_altivec_vcfux' must be a constant integer}}
----------------
sammccall wrote:
> hmm, this doesn't exactly look right to me - we know the type of `vsi` so we should only be considering the signed case I thought.
> 
> However the existing diagnostic for `vui` indicates that it's considering the **signed** case, so I guess this is already broken/bad.
yeah, we should know this is the signed case.

after the macro expansion, the code looks like 

```
_Generic((vsi), vector int
           : (vector float)__builtin_altivec_vcfsx((vector int)(vsi), (index)), vector unsigned int
           : (vector float)__builtin_altivec_vcfux((vector unsigned int)(vsi), (index))); 
```

it is a `GenericSelectionExpr` which contains a switch-like structure (see the AST below), so when we traverse it, we will traverse both `__builtin_altivec_vcfsx` and `__builtin_altivec_vcfux`.

```
`-GenericSelectionExpr 0x9527238 <line:31:3, line:33:88> '__vector float' contains-errors
    |-ImplicitCastExpr 0x9527220 <line:31:12, col:16> '__vector int' <LValueToRValue>
    | `-ParenExpr 0x9526980 <col:12, col:16> '__vector int' lvalue
    |   `-DeclRefExpr 0x9526960 <col:13> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int' non_odr_use_unevaluated
    |-VectorType 0x91923c0 '__vector int' altivec 4
    | `-BuiltinType 0x91296e0 'int'
    |-case  '__vector int' selected
    | |-VectorType 0x91923c0 '__vector int' altivec 4
    | | `-BuiltinType 0x91296e0 'int'
    | `-CStyleCastExpr 0x9526db8 <line:32:14, col:78> '__vector float' contains-errors <Dependent>
    |   `-RecoveryExpr 0x9526d70 <col:28, col:78> '<dependent type>' contains-errors lvalue
    |     |-DeclRefExpr 0x9526bc0 <col:28> '<builtin fn type>' Function 0x95269e8 '__builtin_altivec_vcfsx' '__attribute__((__vector_size__(4 * sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(int)))) int, int)'
    |     |-CStyleCastExpr 0x9526c68 <col:52, col:68> '__vector int' <BitCast>
    |     | `-ImplicitCastExpr 0x9526c50 <col:64, col:68> '__vector int' <LValueToRValue> part_of_explicit_cast
    |     |   `-ParenExpr 0x9526c30 <col:64, col:68> '__vector int' lvalue
    |     |     `-DeclRefExpr 0x9526be0 <col:65> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int'
    |     `-ParenExpr 0x9526cb0 <col:71, col:77> 'const int' lvalue
    |       `-DeclRefExpr 0x9526c90 <col:72> 'const int' lvalue ParmVar 0x95267c8 'index' 'const int'
    `-case  '__vector unsigned int'
      |-VectorType 0x9192700 '__vector unsigned int' altivec 4
      | `-BuiltinType 0x9129780 'unsigned int'
      `-CStyleCastExpr 0x95271f8 <line:33:14, col:87> '__vector float' contains-errors <Dependent>
        `-RecoveryExpr 0x95271b0 <col:28, col:87> '<dependent type>' contains-errors lvalue
          |-DeclRefExpr 0x9527000 <col:28> '<builtin fn type>' Function 0x9526e28 '__builtin_altivec_vcfux' '__attribute__((__vector_size__(4 * sizeof(float)))) float (__attribute__((__vector_size__(4 * sizeof(unsigned int)))) unsigned int, int)'
          |-CStyleCastExpr 0x95270a8 <col:52, col:77> '__vector unsigned int' <BitCast>
          | `-ImplicitCastExpr 0x9527090 <col:73, col:77> '__vector int' <LValueToRValue> part_of_explicit_cast
          |   `-ParenExpr 0x9527070 <col:73, col:77> '__vector int' lvalue
          |     `-DeclRefExpr 0x9527020 <col:74> '__vector int' lvalue Var 0x9526568 'vsi' '__vector int'
          `-ParenExpr 0x95270f0 <col:80, col:86> 'const int' lvalue
            `-DeclRefExpr 0x95270d0 <col:81> 'const int' lvalue ParmVar 0x95267c8 'index' 'const int'
```


> However the existing diagnostic for vui indicates that it's considering the signed case, so I guess this is already broken/bad.

hmm, `vsi` and `vui` have the same type `vector signed int`, so I think there is a typo for `vui`, it should be `vector unsigned int`?


================
Comment at: clang/test/Index/complete-switch.c:9
 
-// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:4:10 %s | FileCheck %s -allow-empty
+// RUN: not %clang_cc1 -fsyntax-only -Xclang -fno-recovery-ast -code-completion-at=%s:4:10 %s | FileCheck %s -allow-empty
 // CHECK-NOT: COMPLETION: foo
----------------
sammccall wrote:
> nit: no need for xclang, this is cc1 already
> 
> I guess this is a crash test (and it also doesn't crash with recovery ast)?
yes, the crash was caused by accesing a null-like ast node. With recovery ast, it is not true any more. 


================
Comment at: clang/test/Sema/__try.c:55
+    // expected-error{{too few arguments to function call, expected 1, have 0}} \
+    // expected-error{{expected ';' after expression}}
   }
----------------
sammccall wrote:
> this seems bad, am I missing something?
agree, `__except` makes it very confusing intuitively. 

what's happening? - while at here, there is no preceding `__try`, clang parses `__execept` as a function call to an implicit function, so it expects a `;` after ` __except (FilterExpression());`

why don't we hit this before? -- because the `FilterExpression()` is invalid (missing an argument), and the whole function call expr is being dropped. With recovery-ast, we preserve the whole function call, then we emit the `;` diagnostic.

not quite sure on how to make it clearer. I think this is not super critical.




================
Comment at: clang/test/Sema/typo-correction.c:56
+  f(THIS_IS_AN_ERROR,       // expected-error {{use of undeclared identifier 'THIS_IS_AN_ERROR'}}
+    afunction(afunction_)); // expected-error {{use of undeclared identifier 'afunction_'}}
 }
----------------
sammccall wrote:
> what's up with this change?
> Do we see the LHS is dependent/contains-errors and then give up on correcting typos in the RHS?
> Should we?
I'd say this aligns with C++ behavior (the inner typo is not correct in C++ mode neither). The reason why it works here is due to the early typo-correction hack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89046