Page MenuHomePhabricator

[AST][RecoveryExpr] Fix a crash on undeduced type.

Authored by hokein on Sep 9 2020, 12:25 AM.



We should not capture the type if the function return type is undeduced.

Diff Detail

Event Timeline

hokein created this revision.Sep 9 2020, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 12:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein requested review of this revision.Sep 9 2020, 12:25 AM
adamcz accepted this revision.Sep 17 2020, 5:20 AM
This revision is now accepted and ready to land.Sep 17 2020, 5:20 AM
sammccall added inline comments.Sep 18 2020, 1:13 AM

Wouldn't we be better to handle undeduced type here rather than in the loop?

Not sure if it practically matters, but given: auto f(); double f(int); f(0,0);
the code in this patch will discard auto and yield double, while treating this as multiple candidates --> unknown return type seems more correct.

hokein added inline comments.Sep 21 2020, 12:01 AM

I feel like yield double is better in your given example -- in this case, double is the only candidate, so it is reasonable to preserve the double type.

sammccall added inline comments.Sep 21 2020, 12:30 AM

There are two overloads, one with double and one with an unknown type. They're both candidates for the function being called, and I'm not sure we have any reason to believe the first is more likely.

Two counterarguments I can think of:

  • the unknown type is probably also double, as overloads tend to have the same return type
  • the cost of being wrong isn't so high, so it's OK to not be totally sure

And maybe there are others... these are probably OK justifications but subtle so I'd like to see a comment tothat effect.

hokein updated this revision to Diff 296120.Mon, Oct 5, 2:14 AM

address comments.

hokein added inline comments.Mon, Oct 5, 2:16 AM

thanks, I think the behavior you suggest is more reasonable -- in the given example, go-to-def will yield two candidates, it is wired to catch one of the return type (not the other).

Addressed it.

sammccall accepted this revision.Mon, Oct 5, 2:30 AM

LG, thanks!

This revision was landed with ongoing or failed builds.Mon, Oct 5, 3:57 AM
This revision was automatically updated to reflect the committed changes.