Closed Bug 1186450 Opened 10 years ago Closed 10 years ago

Fix the template arguments for TraceableHashMaps's GC methods

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

So, the base of the CRTP stack is: template <typename T> class Rooted : RootedBase<T> For TraceableHashMap<K, V, HP, AP, KT, VT>, this means we need to specialize rooted base as: template<typename K, typename V, typename HP, typename AP, typename KT, typename VT> class RootedBase<TraceableHashMap<K, V, HP, AP, KT, VT>> : public MutableTraceableHashMapOperations<TraceableHashMap<K, V, HP, AP, KT, VT>, K, V, HP, AP, KT, VT> This is *quite* long, however, so I used varargs to simplify it to: template<typename... Args> class RootedBase<TraceableHashMap<Args...>> This works since we don't need any members from the actual arg pack, we just need them to pass through. Both gcc and clang are able to properly infer the contents, but MSVC is not. To fix this I added a template template arg with varargs to tell msvc that the first argument is a template with varargs, allowing msvc to properly infer the nested argpack: template<template <...> class TemplateType, typename... Args> class RootedBase<TraceableHashMap<Args...>> This does work, however, the reason this works is not what I thought. Instead of giving more clarity to MSVC, it just changes how specialization works. Specifically, it just broadens the match. This allows the "ambiguous" TraceableHashMap through. Unfortunately it also lets through RootedBase<TraceableVector<Args...>>, which makes it impossible to make a RootedBase specialization for that class. The patch here just removes the template varargs and goes back to direct arg passing to give msvc something easier to digest. The definition here with full args is long, but it's nothing compared to the original where I spelled out all the type names -- that was something like 20 lines long before the {. So, instead of writing out the full argument list, I've used an abbreviated A,B,C,D,E,F form. I think, given that we don't ever use the names -- they're only there so that msvc doesn't get confused -- that it's actually /significantly/ clearer than the alternatives. f+ for this approach from jonco over IRC. I've tested builds locally under msvc and this does indeed compile there. I'll do a full try run later this afternoon once I have more patches queued up for testing.
Attachment #8637259 - Flags: review?(jcoppeard)
Attachment #8637259 - Flags: feedback+
Attachment #8637259 - Flags: review?(jcoppeard) → review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: