Page MenuHomePhabricator

Fix template parameter default args missed if redecled

Authored by erichkeane on Oct 20 2017, 9:03 AM.



This bug was found via self-build on lld, and worked around

The issue is that the 'using' causes the lookup to pick up the
first decl. However, when setting inherited default parameters,
we only update 'forward', not 'backward'. SO, only the newest param
list has all the information about the default arguments.

This patch ensures that the list of parameters we look through checks
the newest decl's template parameter list so it doesn't miss a default.

Diff Detail


Event Timeline

erichkeane created this revision.Oct 20 2017, 9:03 AM
efriedma edited reviewers, added: efriedma; removed: eli.friedman.Oct 20 2017, 8:12 PM
efriedma added a subscriber: cfe-commits.
mstorsjo added inline comments.Oct 21 2017, 6:21 AM
4808 ↗(On Diff #119665)

Typo in "recentdeclaration"

4811 ↗(On Diff #119665)

How does this work if there's another forward declaration missing the parameter later? Is the logic with "most recent" ok, or should it be "most qualified/complete" instead?

erichkeane added inline comments.Oct 22 2017, 9:05 AM
4811 ↗(On Diff #119665)

I'm not sure the case you mean. Any future declarations with fewer parameters would be aspecialization and a different decl, right?

mstorsjo added inline comments.Oct 22 2017, 9:50 AM
4811 ↗(On Diff #119665)

Sorry, I meant a declaration with fewer defaults. E.g. the original forward declaration without defaults again after the actual definition.

E.g. by adding

namespace llvm {
  template<typename T > struct StringSet;

before the last namespace lld.

erichkeane added inline comments.Oct 22 2017, 5:08 PM
4811 ↗(On Diff #119665)

I think thats OK. The code propagates those forward. The problem here is that it does NOT propagate them backwards.

mstorsjo added inline comments.Oct 23 2017, 12:53 AM
4811 ↗(On Diff #119665)

Ok then, then I guess it sounds fine to me, although I'm very much not familiar with this code. So I'm not comfortable with giving it a formal approval.

rnk accepted this revision.Oct 23 2017, 5:43 PM


This revision is now accepted and ready to land.Oct 23 2017, 5:43 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Oct 23 2017, 7:40 PM

I think you forgot to svn add the test case

In D39127#904685, @rnk wrote:

I think you forgot to svn add the test case

Ugg... thanks for the heads up! Added in r316437