Page MenuHomePhabricator

Improve anonymous class heuristic in ClangASTContext::CreateRecordType

Authored by shafik on Aug 13 2019, 3:11 PM.



Currently the heuristic used in ClangASTContext::CreateRecordType to identify an anonymous class is that there is that name is a nullptr or simply a null terminator. This heuristic is not accurate since it will also sweep up unnamed classes and lambdas. The improved heuristic relies on the requirement that an anonymous class must be contained within a class.

The motivator for this fix is that we currently see crashes during code completion inside lambdas and member functions of unnamed classes not within a class. This heuristic fix removes these cases but leaves unnamed classes within a class incorrectly labeled anonymous.

We will fix this problem long-term by implementing DW_AT_export_symbols which will allow us to accurately identify anonymous classes without a heuristic.

Diff Detail


Event Timeline

shafik created this revision.Aug 13 2019, 3:11 PM
aprantl added inline comments.Aug 13 2019, 4:36 PM
4 ↗(On Diff #214933)

Can you rename that test to not contain the word "crash" and instead describe the action being performed?
(CompletionOfLambda or something like that should be fine)

1505 ↗(On Diff #214933)

rename this to has_name

1509 ↗(On Diff #214933)

has_name ? &ast->Idents.get(name) : nullptr

1513 ↗(On Diff #214933)

if (!has_name) {

// In C++ a lambda is also represented as an unnamed class. This is different from 
// an *anonymous class* that the user wrote. Anonymous ...
aprantl added inline comments.Aug 13 2019, 4:40 PM
1521 ↗(On Diff #214933)

// FIXME: An unnamed class within a class is also wrongly recognized as an anonymous struct.

1524 ↗(On Diff #214933)

and move the excuse in the commit message.

It would be really helpful though to include tiny source code examples here to iiustrate the difference between unnames struct and anonymous structs vs. unnamed structs in structs.

Could you expand the test (or add another test) for completing in an anonymous classes? Otherwise this LGTM beside Adrian comments assuming this fixes the crash.

4 ↗(On Diff #214933)

You an actually completely remove the decorators import and the empty [].

shafik updated this revision to Diff 215228.Aug 14 2019, 1:34 PM
shafik marked 7 inline comments as done.

Addressing comments:

  • Rewording comments
  • Moving test location
  • Adding test case

@aprantl @teemperor Good catches, I have updated the review.

aprantl added inline comments.Aug 14 2019, 1:44 PM
1505 ↗(On Diff #215228)

The condition needs to be reversed now to match the new name.

1516 ↗(On Diff #215228)

// anonymous class (GNU/MSVC extension)

1509 ↗(On Diff #214933)

and then here, too.

aprantl added inline comments.Aug 14 2019, 1:53 PM
1505 ↗(On Diff #215228)

bool has_name = (name && name[0])

1505 ↗(On Diff #215228)

Even more elegant would be changing the function to take a StringRef, but you don't have to do that....

1511 ↗(On Diff #215228)

if (!has_name)

shafik updated this revision to Diff 215243.Aug 14 2019, 2:13 PM
shafik marked 6 inline comments as done.

Fixing has_name to better reflect the condition.

aprantl accepted this revision.Aug 14 2019, 2:27 PM
This revision is now accepted and ready to land.Aug 14 2019, 2:27 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2019, 3:29 PM