713082 - Remove Util.h

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

<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

Comment 2

[Biru [:poiru]]

Attachment: Part 1: Remove unnecessary Util.h includes

Comment 3

[Biru [:poiru]]

Attachment: Part 2: Merge Util.h functions into Array.h

Merged Util.h into Array.h as per bug 932840 comment #0.

Comment 4

[Biru [:poiru]]

Attachment: Part 3: Remove unnecessary Array.h includes

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=61011958b264

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8333508 [details] [diff] [review]
Part 1: Remove unnecessary Util.h includes

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

rs=me

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: A fair bit of adding ArrayUtils.h, replacing Util.h uses with it when possible

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Biru [:poiru]]

Attachment: Part 2: Rename Util.h to ArrayUtils.h

(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.

Comment 10

[Biru [:poiru]]

Attachment: Part 1: Remove unnecessary Util.h includes

Rebased patch.

Comment 11

[Biru [:poiru]]

Attachment: Part 3: Remove unnecessary Array.h includes

Rebased patch.

Pushed the latest iteration to try: https://tbpl.mozilla.org/?tree=Try&rev=330fc145ec9b

Comment 12

[Jeff Walden [:Waldo]]

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.

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Biru [:poiru]]

Attachment: Part 1: Remove unnecessary Util.h includes

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.

Comment 15

[Biru [:poiru]]

Attachment: Part 2: Rename Util.h to ArrayUtils.h

Rebased patch and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d21dbf8732d7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dfdc31408f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0776db3b102

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dfdc31408f9
https://hg.mozilla.org/mozilla-central/rev/e0776db3b102