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

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 631969 [details] [diff] [review]
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.
Attachment #631969 - Flags: review?(Ms2ger)
(Assignee)

Updated

5 years ago
Attachment #631969 - Flags: review?(Ms2ger) → review?(khuey)
> 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 on attachment 631969 [details] [diff] [review]
use typename everywhere

I'm going to let you work this out with bz.
Attachment #631969 - Flags: review?(khuey) → review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
(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 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.  ;)
Attachment #631969 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

5 years ago
Summary: Consistently use |typename| for type template params in BindingUtil.h → Use typename for type parameters of template template parameters in BindingUtil.h
(Assignee)

Comment 5

5 years ago
Created attachment 632306 [details] [diff] [review]
Use typename

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

Carrying forward r=bz
Attachment #632306 - Flags: review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f03bb92c3140
Assignee: nobody → bjacob
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f03bb92c3140
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.