Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: poiru)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

7 years ago
<Waldo> incidentally, Util.h as a dumping ground should go away, and if you want to file bugs on splitting stuff up some -- DebugOnly should be in DebugOnly.h, PointerRangeSize and Array* should be in something like ArrayUtils.h or whatever, MOZ_ALIGNOF should be in Alignment.h, and so on, that'd be helpful
<Waldo> dumping ground is maybe friendlier to using the stuff
<Waldo> but for learning where things are, it's hard to use that strategy if we want the code comments to be the authoritative documentation
Depends on: 820570
(Reporter)

Updated

6 years ago
Depends on: 904110
(Assignee)

Updated

5 years ago
Duplicate of this bug: 932840
(Assignee)

Updated

5 years ago
Assignee: nobody → birunthan
(Assignee)

Comment 2

5 years ago
Attachment #8333508 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Merged Util.h into Array.h as per bug 932840 comment #0.
Attachment #8333510 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 8333508 [details] [diff] [review]
Part 1: Remove unnecessary Util.h includes

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

rs=me
Attachment #8333508 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8333510 [details] [diff] [review]
Part 2: Merge Util.h functions into Array.h

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

That bug comment was misguided.  Right now Array.h exposes a class that's a rough subset of C++11's <array> header.  Whenever STL support advances far enough to pick it up, I'm pretty sure we'll want to switch over.  So moving stuff into Array.h isn't the right move -- we want a new header for this.  I've been carrying along a patch for awhile now that does that -- been a long time since I even attempted to build it, but it'll probably get you a long ways toward that sort of fix.  Once second, I'll upload what I have here.
Attachment #8333510 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8333511 [details] [diff] [review]
Part 3: Remove unnecessary Array.h includes

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

It's hard to comment one way or another on this, given the previous patch isn't what's desired.  Flag me again if this is relevant, or (probably better) post this patch rebased atop the partial work I just posted.
Attachment #8333511 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 9

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> That bug comment was misguided.  Right now Array.h exposes a class that's a
> rough subset of C++11's <array> header.  Whenever STL support advances far
> enough to pick it up, I'm pretty sure we'll want to switch over.  So moving
> stuff into Array.h isn't the right move -- we want a new header for this. 

Thanks, updated the patch to do this.
Attachment #8333510 - Attachment is obsolete: true
Attachment #8338177 - Attachment is obsolete: true
Attachment #8340613 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #8340613 - Attachment description: Rename Util.h to ArrayUtils.h → Part 2: Rename Util.h to ArrayUtils.h
(Assignee)

Comment 10

5 years ago
Rebased patch.
Attachment #8333508 - Attachment is obsolete: true
Attachment #8340614 - Flags: review+
(Assignee)

Comment 11

5 years ago
Rebased patch.

Pushed the latest iteration to try: https://tbpl.mozilla.org/?tree=Try&rev=330fc145ec9b
Attachment #8333511 - Attachment is obsolete: true
Attachment #8340615 - Flags: review?(jwalden+bmo)
Comment on attachment 8340613 [details] [diff] [review]
Part 2: Rename Util.h to ArrayUtils.h

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

::: mfbt/ArrayUtils.h
@@ +13,4 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Types.h"

Get rid of this one, use #include <stddef.h> instead, in its own section underneath the Attributes.h #include.  size_t is the only thing we need, we shouldn't over-include.

@@ +16,5 @@
>  #include "mozilla/Types.h"
>  
>  #ifdef __cplusplus
>  
>  #include "mozilla/Alignment.h"

I think you can get rid of this #include as well.
Attachment #8340613 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8340615 [details] [diff] [review]
Part 3: Remove unnecessary Array.h includes

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

We don't want this change -- see below for why.  Did you apply similar criteria for determining when a Util.h #include was unnecessary?  If so, we probably don't want that patch either.

::: js/src/jit/BacktrackingAllocator.h
@@ -6,5 @@
>  
>  #ifndef jit_BacktrackingAllocator_h
>  #define jit_BacktrackingAllocator_h
>  
> -#include "mozilla/Array.h"

This isn't right, this header uses mozilla::Array directly.  It's not okay for a header to depend on the headers it includes, to pick up a declaration or definition -- it should include that header itself, as happens here.

::: js/src/jit/LIR.h
@@ -9,5 @@
>  
>  // This file declares the core data structures for LIR: storage allocations for
>  // inputs and outputs, as well as the interface instructions must conform to.
>  
> -#include "mozilla/Array.h"

Same here.

::: js/src/jit/LiveRangeAllocator.h
@@ -6,5 @@
>  
>  #ifndef jit_LiveRangeAllocator_h
>  #define jit_LiveRangeAllocator_h
>  
> -#include "mozilla/Array.h"

Same.

::: js/src/jit/Registers.h
@@ -6,5 @@
>  
>  #ifndef jit_Registers_h
>  #define jit_Registers_h
>  
> -#include "mozilla/Array.h"

Same.
Attachment #8340615 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 14

5 years ago
Rebased patch and incorporated suggestions in comment #12.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Comment on attachment 8340615 [details] [diff] [review]
> Part 3: Remove unnecessary Array.h includes
> 
> Review of attachment 8340615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We don't want this change -- see below for why.  Did you apply similar
> criteria for determining when a Util.h #include was unnecessary?  If so, we
> probably don't want that patch either.

Whoops, thanks for catching that -- I'll mark that the Part 3 patch as obsolete. I didn't update my script properly to catch a bare "Array" -- the other patches should be fine.
Attachment #8340614 - Attachment is obsolete: true
Attachment #8340615 - Attachment is obsolete: true
Attachment #8344144 - Flags: review+
(Assignee)

Comment 15

5 years ago
Rebased patch and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d21dbf8732d7
Attachment #8340613 - Attachment is obsolete: true
Attachment #8344145 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6dfdc31408f9
https://hg.mozilla.org/mozilla-central/rev/e0776db3b102
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.