Page MenuHomePhabricator

Replace function template (plus explicit specializations) by non-template overloads.
ClosedPublic

Authored by tkoeppe on Mar 5 2019, 2:28 PM.

Details

Summary

For the design in question, overloads seem to be a much simpler and less subtle solution.

This removes ODR issues, and errors of the kind where code that uses the specialization in question will accidentally and erroneously specialize the primary template. This only "works" by accident; the program is ill-formed NDR.

(Found with -Wundefined-func-template.)

Diff Detail

Repository
rL LLVM

Event Timeline

tkoeppe created this revision.Mar 5 2019, 2:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 2:28 PM
chandlerc added inline comments.Mar 5 2019, 2:37 PM
lib/Target/Hexagon/RDFGraph.h
941–942 ↗(On Diff #189400)

Should probably move the comment below up here and explain more what this is doing. IE: that this template is explicitly specialized for specific types only, etc.

944–945 ↗(On Diff #189400)

FWIW, I don't think you need to add the forward reference to RDFLiveness.h here.

tkoeppe updated this revision to Diff 189401.Mar 5 2019, 2:44 PM

Moved comment up to the declaration of the primary template and removed mention of other specializations in different headers.

tkoeppe marked 2 inline comments as done.Mar 5 2019, 2:45 PM
tkoeppe added inline comments.
lib/Target/Hexagon/RDFGraph.h
944–945 ↗(On Diff #189400)

I omitted this when I moved the comment up.

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

They could, but...

a) I wanted to make a minimal change and respect the original design,
b) Richard Smith told me offline that he had also considered it but doesn't much care either way, and
c) I was suspecting some attempt to speed up compilation by keeping the overload set minimal, which I didn't want to second-guess lightly.

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

Is there any reason this needs to be a template -- can't these just be changed to function overloads, instead of template specializations?

I'll second this - and I'm happy to take ownership (if it's needed) of the decision. This code came in with this specialization approach, and I'm going to guess it wasn't for any particularly nuanced reason & is fine to change to overloads as would be more common here.

The only reason I can think of is that we wanted to keep the overload set small (or trivial), but I don't know if that's significant. (Richard said he doesn't care either way.)

What shall we do now? Would you like to take this change and then refactor, or immediately change this to overloads?

I'd just immediately switch to overloads as a means to fix the original issue rather than do a two-step.

Ah, another reason to prefer templates might be that templates deduce, and overloads convert. With the template, you can be sure that there aren't any accidental conversions (which would hit the deleted primary template).

Do you think that matters? If so, we can preserve that behaviour by retaining the deleted primary template (but NOT defining any explicit specializations).

tkoeppe updated this revision to Diff 189996.Mar 9 2019, 2:58 PM
tkoeppe retitled this revision from Add declarations of explicit specializations and make primary template deleted to Replace function template (plus explicit specializations) by non-template overloads..
tkoeppe edited the summary of this revision. (Show Details)

Changed function template plus specializations to function overloads.

dblaikie accepted this revision.Mar 9 2019, 5:13 PM

Looks good to me, thanks

Could you make sure you've run this change through clang-format before submitting? (Or if you do not have commit rights, I can format/commit it for you)

This revision is now accepted and ready to land.Mar 9 2019, 5:13 PM

Could you make sure you've run this change through clang-format before submitting? (Or if you do not have commit rights, I can format/commit it for you)

I'm pretty sure I don't have those; could you please? Thanks a lot!

(I take it that tests have already run and passed for this?)

This revision was automatically updated to reflect the committed changes.