Remove |using namespace mozilla;| from AsmJS.cpp

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
With both mozilla::Vector and js::Vector existing, opening both js and mozilla namespaces makes for prefixing sadtimes.  Use specific things from mozilla rather than import the entire thing wholesale.
(Assignee)

Comment 1

5 years ago
Created attachment 778227 [details] [diff] [review]
Patch
Attachment #778227 - Flags: review?(terrence)

Comment 2

5 years ago
Hah, I have literally the same patch in my queue (plus I fixed AsmJS.h so AsmJS.cpp could #include AsmJS.h first).  Want me to post that one?
(Assignee)

Comment 3

5 years ago
Created attachment 778233 [details] [diff] [review]
asmjs-bootleg.diff

...not if I've written it and go to post it first.  Or something.  :-)  I imagine this one might slightly differ from your version of it in irrelevant details, but it's doubtless nigh-identical.
Attachment #778233 - Flags: review?(luke)

Comment 4

5 years ago
Comment on attachment 778233 [details] [diff] [review]
asmjs-bootleg.diff

Whoaaa.  http://www.youtube.com/watch?v=OT4B-NJUcZE
Attachment #778233 - Flags: review?(luke) → review+

Comment 5

5 years ago
Well then, I have to ask: do you also have a patch in your queue that greedily partakes in the use of C++11 'auto' in AsmJS.cpp because... it seems like we can now?

Btw, is there any reason *you* know of not to start using 'auto' in SM?
(Assignee)

Comment 6

5 years ago
Whoa there Tonto, slow down.  :-D  I think there's a fair argument that spewing auto "everywhere" hurts readability precisely because it obscures types in places.  So I think using it *judiciously* is key to using it well.  I haven't looked into it at all, myself, to say where/what is definitely awesome, what we might want to stick to explicitness, etc.  Then again, it's possible I'm overrating the concern there, so who knows.

The .platform thread on what we can apparently use Real Soon Now, and the extent of it, actually surprised me quite a lot.  Good stuff coming!

Feel free to poach the review on the first patch if you want, while you're at it.  :-)

Comment 7

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
Definitely agree on not spewing it "everywhere", but there are tons of cases where the type on the left is uninteresting.  When the first patch lands to use 'auto', I was going to start an email thread to engine-internals on the subject.

> The .platform thread on what we can apparently use Real Soon Now, and the
> extent of it, actually surprised me quite a lot.  Good stuff coming!

I think we can use 'auto' now, though, right?

Comment 8

5 years ago
Comment on attachment 778227 [details] [diff] [review]
Patch

Poached.
Attachment #778227 - Flags: review?(terrence) → review+
(Assignee)

Comment 10

5 years ago
It sounds like yes, almost, but I'm not confident enough in my reading of the thread to assert that we definitely can.
https://hg.mozilla.org/mozilla-central/rev/02e9ac646def
https://hg.mozilla.org/mozilla-central/rev/e4ed450913fe
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.