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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang/include/clang/Basic/LangOptions.def:153
 COMPATIBLE_LANGOPT(RecoveryASTType, 1, 0, "Preserve the type in recovery expressions")
+COMPATIBLE_LANGOPT(CDependence, 1, 0, "Build dependent AST nodes in C for better error recovery")
 
----------------
hokein wrote:
> sammccall wrote:
> > 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?
> 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.
> 
> 
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())`


================
Comment at: clang/include/clang/Sema/Sema.h:5169
+  /// In C++, we need to perform unqualified lookups when creating a dependent
+  /// binary operator, thus this should be only used when the unqalified lookup
+  /// found nothing.
----------------
nit: unqualified
nit: remove "results"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84226