Last Comment Bug 763604 - Use typename for type parameters of template template parameters in BindingUtil.h
: Use typename for type parameters of template template parameters in BindingUt...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-11 11:49 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-13 05:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use typename everywhere (8.53 KB, patch)
2012-06-11 11:49 PDT, Benoit Jacob [:bjacob] (mostly away)
bzbarsky: review+
Details | Diff | Splinter Review
Use typename (2.11 KB, patch)
2012-06-12 10:11 PDT, Benoit Jacob [:bjacob] (mostly away)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-06-11 11:49:09 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-06-11 13:17:49 PDT
> 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 [:khuey] (khuey@mozilla.com) 2012-06-11 13:19:11 PDT
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) 2012-06-11 13:31:24 PDT
(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 [:bz] 2012-06-11 13:37:50 PDT
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) 2012-06-12 10:11:18 PDT
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
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-12 14:22:48 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f03bb92c3140
Comment 7 Ed Morley [:emorley] 2012-06-13 05:58:58 PDT
https://hg.mozilla.org/mozilla-central/rev/f03bb92c3140

Note You need to log in before you can comment on or make changes to this bug.