[PATCH] D78350: [AST] Build recovery expressions by default for C++.

Thu Apr 30 03:57:49 PDT 2020

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Code LG, let's chat before landing though - want to understand the state of the internal testing.

================
Comment at: clang/include/clang/Basic/LangOptions.def:151

-COMPATIBLE_LANGOPT(RecoveryAST, 1, 0, "Preserve expressions in AST when encountering errors")
+COMPATIBLE_LANGOPT(RecoveryAST, 1, CPlusPlus, "Preserve expressions in AST when encountering errors")

----------------
hokein wrote:
> rsmith wrote:
> > Does this work? I would expect that we set all the options to the defaults at the same time, so this just sets this option to 0 (the default for `CPlusPlus`). If so, it'd be clearer to explicitly write that default here.
> I'm following the scheme of other fields, e.g. `WChar`, the real initialization is done in `CompilerInvocation.cpp`. 
> 
> I think writing `CPlusPlus` is a bit clearer here, which indicates this option is associated with CPlusPlus flag.
I think I suggested this in the past (based on precedent) but agree with Richard it's probably more confusing than enlightening.

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2892
     Diags.Report(diag::warn_fe_concepts_ts_flag);
   Opts.RecoveryAST =
+      Args.hasFlag(OPT_frecovery_ast, OPT_fno_recovery_ast, Opts.CPlusPlus);
----------------
you could add a comment here like "Recovery AST still heavily relies on dependent-type machinery" or something.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78350