763604 - Use typename for type parameters of template template parameters in BindingUtil.h

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: use typename everywhere

What I really wanted to fix was this:

  template<class T, template<class> class SmartPtr>

which is IMO more readable as:

  template<typename T, template<typename> class SmartPtr>

Also, this file already uses typename for certain type template params, and sometimes mixes typename and class in the same line, like:

  template <class T, typename U>

Maybe this was intentional as this T can't be a non-class type, but in that case I'd argue that this gives the wrong impression that this would guarantee this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Maybe this was intentional as this T can't be a non-class type

It sort of was, yes.  In particular, it needs to be a type for which PrototypeIDMap has a specialization, and those are only generated for (some) classes.

I don't object to using "typename" here, I guess, but I did slightly like the "self-documenting" aspect of "class" there.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 631969 [details] [diff] [review]
use typename everywhere

I'm going to let you work this out with bz.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky (:bz) from comment #1)
> > Maybe this was intentional as this T can't be a non-class type
> 
> It sort of was, yes.  In particular, it needs to be a type for which
> PrototypeIDMap has a specialization, and those are only generated for (some)
> classes.
> 
> I don't object to using "typename" here, I guess, but I did slightly like
> the "self-documenting" aspect of "class" there.

My main concern with this is that most of the time when class is used instead of typename, it's interchangeably, so the uses of class here don't self-document much for the average reader (who doesn't know about your intention here). On the other hand, the cost in terms of readability in 

  template<class T, template<class> class SmartPtr>

is real IMO.

But this isn't very important so I guess I'll WONTFIX this if we can't agree now that this is a clear improvement.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631969 [details] [diff] [review]
use typename everywhere

Oh, I agree the "template<typename> class SmartPtr" bit is much better, and we should do that part.  And like I said, I can live with the rest...  

r=me, though if you want to just fix the "template<typename> class SmartPtr" cases that would be fine by me too.  ;)

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Use typename

Let's go for minimal churn and only do the change that everybody agrees is an improvement.

Carrying forward r=bz

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f03bb92c3140

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f03bb92c3140