Closed Bug 1464794 Opened 6 years ago Closed 6 years ago

Remove 'using mozilla::XYZ' from header files

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Attached patch bug1464794.patch (obsolete) — Splinter Review
- Remove "using mozilla::{Forward,Move}" from js/src/ds/OrderedHashTable.h
- Remove "using mozilla::FloatingPoint" from js/src/jit/MacroAssembler.h
- Remove "using mozilla::DebugOnly" from js/src/jit/arm/MacroAssembler-arm.h
- And then handle the fallout of the removal in lots of places.
Attachment #8981129 - Flags: review?(jwalden+bmo)
Comment on attachment 8981129 [details] [diff] [review]
bug1464794.patch

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

Could you add |using mozilla::Move;| and similar to the top of the non-header files, then not make the other changes to those files?  Makes for lots smaller patches, and this is how we tend to write things now these days anyway.  A quick ack on js/src suggests 446 instances of "using mozilla::", to get a sense of things.
Attachment #8981129 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] from comment #3)
> Could you add |using mozilla::Move;| and similar to the top of the
> non-header files, then not make the other changes to those files?

Don't we have the same problem with cpp files, due to the unified builds?
(In reply to Nicolas B. Pierron [:nbp] {backlog: 36} from comment #4)
> Don't we have the same problem with cpp files, due to the unified builds?

Somewhat yes, and that's why unified builds are somewhat stoopid.  But that's why we have a non-unified build on treeherder.

And the problem is qualitatively different for headers versus .cpp files: headers deliberately infect other code, .cpp do not except incidentally.  If we were really serious about that solution, we would remove all |using| declarations at namespace scope and we would fully qualify everything that would otherwise depend on them.  I don't see us doing that, and I'm not sure this is the bug to have that argument in anyway.
Attached patch bug1464794.patch (obsolete) — Splinter Review
Updated per review comments, carrying r+.
Attachment #8981129 - Attachment is obsolete: true
Attachment #8982446 - Flags: review+
Needs rebasing.
Keywords: checkin-needed
Attached patch bug1464794.patchSplinter Review
Rebased patch, carrying r+.
Attachment #8982446 - Attachment is obsolete: true
Attachment #8982996 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc60824e00d
Remove 'using mozilla::*' from header files in js/src. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4cc60824e00d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: