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

[tkoeppe]

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.)

[chandlerc]

lib/Target/Hexagon/RDFGraph.h

941–942

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

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

[tkoeppe]

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

[tkoeppe]

lib/Target/Hexagon/RDFGraph.h

944–945

I omitted this when I moved the comment up.

[jyknight]

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

[tkoeppe]

In D58998#1419205, @jyknight wrote:

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.

[dblaikie]

In D58998#1419205, @jyknight wrote:

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.

[tkoeppe]

In D58998#1421799, @dblaikie wrote:

In D58998#1419205, @jyknight wrote:

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?

[dblaikie]

In D58998#1422074, @tkoeppe wrote:

In D58998#1421799, @dblaikie wrote:

In D58998#1419205, @jyknight wrote:

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.

[tkoeppe]

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]

Changed function template plus specializations to function overloads.

[dblaikie]

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)

[tkoeppe]

In D58998#1423838, @dblaikie wrote:

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?)

lib/Target/Hexagon/RDFGraph.h

     return MM;
   }
 
-  template <typename T> struct Print;
-  template <typename T>
-  raw_ostream &operator<< (raw_ostream &OS, const Print<T> &P);
-
   template <typename T>
   struct Print {
     Print(const T &x, const DataFlowGraph &g) : Obj(x), G(g) {}

       : Print<NodeAddr<T>>(x, g) {}
   };
 
+  template <typename T>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<T> &P) = delete;
+
+  // Explicit specializations in RDFGraph.cpp. There is an additional explicit
+  // specialization in RDFLiveness.cpp; it is declared in RDFLiveness.h.
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterRef> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeId> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<DefNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<UseNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<PhiUseNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<RefNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeList> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeSet> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<PhiNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<StmtNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<InstrNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<BlockNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<NodeAddr<FuncNode*>> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterSet> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<RegisterAggr> &P);
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<DataFlowGraph::DefStack> &P);
+
 } // end namespace rdf
 
 } // end namespace llvm

lib/Target/Hexagon/RDFLiveness.h

         unsigned Nest, unsigned MaxNest);
   };
 
+  // Explicit specialization in RDFLiveness.cpp; primary template in RDFGraph.h.
+  template<>
+  raw_ostream &operator<< (raw_ostream &OS, const Print<Liveness::RefMap> &P);
+
 } // end namespace rdf
 
 } // end namespace llvm