This is an archive of the discontinued LLVM Phabricator instance.

[AST] Build recovery expressions by default for C++.
ClosedPublic

Authored by hokein on Apr 17 2020, 2:09 AM.

Details

Summary

Reland https://reviews.llvm.org/D76696.
All known crashes have been fixed, another attemption.

Plan:
we have rolled out this to a subset of internal users, I'd wait for one
or two weeks to make sure no new crashes.

Diff Detail

Event Timeline

hokein created this revision.Apr 17 2020, 2:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2020, 2:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein updated this revision to Diff 258268.Apr 17 2020, 2:20 AM

fix clangd test.

@ebevhan, @hubert.reinterpretcast, the patch is based on fd7a34186137168064ffe2ca536823559b92d939, it should contain all the fixes.
it would be nice if you can test it again in your downstream clang. Thanks!

@ebevhan, @hubert.reinterpretcast, the patch is based on fd7a34186137168064ffe2ca536823559b92d939, it should contain all the fixes.
it would be nice if you can test it again in your downstream clang. Thanks!

Got it. I'll put together a build.

hokein marked 8 inline comments as done.Apr 28 2020, 12:04 AM

@ebevhan, @hubert.reinterpretcast, the patch is based on fd7a34186137168064ffe2ca536823559b92d939, it should contain all the fixes.
it would be nice if you can test it again in your downstream clang. Thanks!

Got it. I'll put together a build.

Thank you! Look forward to the result. The patch should work on the latest master (b73290be9fa413d8bc426512cdf4fa01bc005954).

clang/test/CXX/class.access/p4.cpp
1
clang/test/CXX/special/class.ctor/p5-0x.cpp
1
clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
23
clang/test/SemaCXX/constant-expression-cxx11.cpp
880
clang/test/SemaCXX/cxx0x-deleted-default-ctor.cpp
1
clang/test/SemaCXX/for-range-dereference.cpp
88
clang/test/SemaCXX/virtual-base-used.cpp
43
clang/test/SemaObjCXX/arc-0x.mm
118

Got it. I'll put together a build.

Thank you! Look forward to the result. The patch should work on the latest master (b73290be9fa413d8bc426512cdf4fa01bc005954).

Looks clean on my end.

rsmith added a subscriber: rsmith.Apr 28 2020, 11:47 PM
rsmith added inline comments.
clang/include/clang/Basic/LangOptions.def
151 ↗(On Diff #260552)

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.

hokein marked an inline comment as done.Apr 30 2020, 3:02 AM

Got it. I'll put together a build.

Thank you! Look forward to the result. The patch should work on the latest master (b73290be9fa413d8bc426512cdf4fa01bc005954).

Looks clean on my end.

Great, thanks!

clang/include/clang/Basic/LangOptions.def
151 ↗(On Diff #260552)

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.

sammccall accepted this revision.Apr 30 2020, 3:50 AM

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

clang/include/clang/Basic/LangOptions.def
151 ↗(On Diff #260552)

I think I suggested this in the past (based on precedent) but agree with Richard it's probably more confusing than enlightening.

clang/lib/Frontend/CompilerInvocation.cpp
2889

you could add a comment here like "Recovery AST still heavily relies on dependent-type machinery" or something.

This revision is now accepted and ready to land.Apr 30 2020, 3:50 AM
hokein updated this revision to Diff 263647.May 13 2020, 1:57 AM

rebase to master

hokein updated this revision to Diff 268105.Jun 3 2020, 2:21 AM

rebase to master.

hokein updated this revision to Diff 269859.Jun 10 2020, 8:13 AM
hokein marked 2 inline comments as done.

rebase to master and address comments.

ok, the current status seems quite stable now. We rolled it out to all our internal users for a while, didn't see big crashes. I'm going to land it.

This revision was automatically updated to reflect the committed changes.
clang/test/SemaCXX/for-range-dereference.cpp