Closed Bug 932454 Opened 7 years ago Closed 7 years ago

Make Atomics.h compilable with clang on windows

Categories

(Core :: MFBT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(1 file)

MSVC is sloppy with typedefs leaking. Clang is more strict.
Attachment #824214 - Flags: review?(froydnj)
Attachment #824214 - Flags: review?(froydnj) → review?(nfroyd)
Attachment #824214 - Attachment is patch: true
Attachment #824214 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 824214 [details] [diff] [review]
Make Atomics.h compilable with clang on windows

Review of attachment 824214 [details] [diff] [review]:
-----------------------------------------------------------------

Curiosity got me, here's an r+ if you want it, this being trivial.  :-)  I guess none of this code gets hit with non-Windows clang because every place that uses clang, has an <atomics> that we use?

::: mfbt/Atomics.h
@@ +702,5 @@
>  
>  template<typename T, MemoryOrdering Order>
>  struct IntrinsicMemoryOps : public IntrinsicBase<T>
>  {
> +    typedef typename IntrinsicBase<T>::ValueType ValueType;

using IntrinsicBase<T>::ValueType; is shorter, if you want.
Attachment #824214 - Flags: review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Comment on attachment 824214 [details] [diff] [review]
> Make Atomics.h compilable with clang on windows
> 
> Review of attachment 824214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Curiosity got me, here's an r+ if you want it, this being trivial.  :-)  I
> guess none of this code gets hit with non-Windows clang because every place
> that uses clang, has an <atomics> that we use?

This code is #ifdef MSVC

> ::: mfbt/Atomics.h
> @@ +702,5 @@
> >  
> >  template<typename T, MemoryOrdering Order>
> >  struct IntrinsicMemoryOps : public IntrinsicBase<T>
> >  {
> > +    typedef typename IntrinsicBase<T>::ValueType ValueType;
> 
> using IntrinsicBase<T>::ValueType; is shorter, if you want.

I believe I tried that first and didn't seem to work.
Comment on attachment 824214 [details] [diff] [review]
Make Atomics.h compilable with clang on windows

Review of attachment 824214 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, since the other intrinsic implementations need something similar.
Attachment #824214 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/19f77c75ac93
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.