Page MenuHomePhabricator

Fix type printing of array template args
Needs ReviewPublic

Authored by reikdas on Aug 5 2017, 5:58 PM.


constexpr const char kEta[] = "Eta";
template <const char*, typename T> class Column {};
using quick = Column<kEta,double>;

void lookup() {
   quick c1;;

emits error: no member named 'ls' in 'Column<&kEta, double>' Attached patch fixes the printed type name by not printing the ampersand for array types.

It passes check-clang for me.

If this change is acceptable: where do you want a test to go?

Diff Detail

Event Timeline

karies created this revision.Aug 5 2017, 5:58 PM
rjmccall edited edge metadata.Aug 6 2017, 11:06 PM

I think test/SemaTemplate/temp_arg_nontype.cpp is probably the most appropriate place.

Unfortunately, I don't think the fix is totally correct. Both &kEta and kEta are valid template arguments, just for template parameters of different types. You need to check getParamTypeForDecl(), and, unfortunately, the possibility of qualification conversions will make the check somewhat annoying.

On the plus side, if you're checking the type, you can also resolve that FIXME about reference arguments. I think this code probably predates the inclusion of a parameter type in Decl TemplateArguments.

karies updated this revision to Diff 109963.Aug 7 2017, 2:57 AM

Only write out an ampersand if needed to make parameter and argument match types.
Increase test coverage (including a text suggested by @rjmccall using an array param) and adjust existing tests' expected diagnostics to follow this change.

karies updated this revision to Diff 109965.Aug 7 2017, 3:00 AM

...and remove FIXME.

Unfortunately, qualification conversions can be nested, e.g. a char** can be converted to a const char * const *. I think it would probably be easiest to just count the nesting depths of arrays.

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 10:05 PM
reikdas commandeered this revision.Mon, Nov 2, 3:00 AM
reikdas added a reviewer: karies.
reikdas updated this revision to Diff 302234.Mon, Nov 2, 3:02 AM

Addresses @rjmccall 's comment