Last Comment Bug 798914 - Consolidate MallocSizeOf typedefs
: Consolidate MallocSizeOf typedefs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Catalin Iacob
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-07 03:48 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-07-01 05:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1 (1.57 KB, patch)
2013-06-16 06:15 PDT, Catalin Iacob
no flags Details | Diff | Splinter Review
Part 2 - use MallocSizeOf in chromium (3.69 KB, patch)
2013-06-16 06:16 PDT, Catalin Iacob
no flags Details | Diff | Splinter Review
WIP, lacks includes Part 3 - replace nsMallocSizeOfFun with MallocSizeOf (294.15 KB, patch)
2013-06-16 06:18 PDT, Catalin Iacob
no flags Details | Diff | Splinter Review
Part 1 (1.68 KB, patch)
2013-06-20 14:21 PDT, Catalin Iacob
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2 (3.69 KB, patch)
2013-06-20 14:23 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 3 (2.70 KB, patch)
2013-06-20 14:24 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 4 (72.26 KB, patch)
2013-06-20 14:26 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 5 (380.67 KB, patch)
2013-06-20 14:27 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 1 (1.73 KB, patch)
2013-06-23 05:05 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 2 (3.66 KB, patch)
2013-06-23 05:06 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 3 (2.71 KB, patch)
2013-06-23 05:06 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 4 (68.22 KB, patch)
2013-06-23 05:07 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Part 5 (381.05 KB, patch)
2013-06-23 05:10 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review
Followup, don't include MemoryReporting.h before a .cpp's own header (12.54 KB, patch)
2013-06-30 07:39 PDT, Catalin Iacob
n.nethercote: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-10-07 03:48:27 PDT
We've now got one in JS, one in XPCOM, one for our Chromium fork, and one for XPT. All of these depend on MFBT, so we should introduce a mozilla::MallocSizeOf there, and kill all the others.
Comment 1 Nicholas Nethercote [:njn] 2012-10-07 15:16:12 PDT
I'm happy to do this.  My only question is:  where in mfbt/ should this live?  Types.h?  Util.h?  A new file?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-10-08 04:52:23 PDT
I'm pretty sure Waldo would argue for a new file :)
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-08 09:40:45 PDT
Yup.  MemoryReporting.h maybe?  I don't know a whole lot about the memory reporting infrastructure, or exposed functionality/stuff, but I assume there's more to it than just a single typedef, and a new header for all of it would make sense.
Comment 4 Nicholas Nethercote [:njn] 2012-10-08 17:36:53 PDT
It would only contain a single typedef.  I'm still happy to have a single file for it, though.
Comment 5 Catalin Iacob 2013-06-16 04:34:16 PDT
I started doing this.

One issue is that xpt_arena.h is also meant to be included from C not just C++ so if the new MallocSizeOfFun is in namespace mozilla it can't be used by xpt_arena.h.

So do I (the options below are ordered by personal preference):
1. put MallocSizeOfFun in the mozilla namespace and don't reuse it in xpt?
2. put MallocSizeOfFun in the mozilla namespace only for C++ and in the global namespace for C? If so should it be named differently for C, something like mozMallocSizeOfFun?
3. don't put MallocSizeOfFun in the mozilla namespace and reuse it everywhere, including xpt?
Comment 6 Catalin Iacob 2013-06-16 05:25:08 PDT
Actually, nsMallocSizeOfFun is defined in xpcom/base/nscore.h and this one also needs to be usable as a C header so I can't just include a C++ only MemoryReporting.h in it.

This means I would need to push the include of the new header down into all files that currently use nsMallocSizeOfFun.
Comment 7 Catalin Iacob 2013-06-16 06:15:21 PDT
Created attachment 763229 [details] [diff] [review]
part1
Comment 8 Catalin Iacob 2013-06-16 06:16:34 PDT
Created attachment 763230 [details] [diff] [review]
Part 2 - use MallocSizeOf in chromium
Comment 9 Catalin Iacob 2013-06-16 06:18:06 PDT
Created attachment 763231 [details] [diff] [review]
WIP, lacks includes Part 3 - replace nsMallocSizeOfFun with MallocSizeOf
Comment 10 Catalin Iacob 2013-06-16 06:22:14 PDT
I've attached what I have so far so that I can get some validation for the general approach.

For the third patch I did the replacement automatically with sed but I still need to go through the files and include the new file everywhere to get everything to build. 

BTW, is it ok to add the same include to a .h and a .cpp file even though it's not really needed? It would be a lot easier to just add an #include "mozilla/MemoryReporting.h" to all files that I modified instead of adding it only to the files that really need it.
Comment 11 Nicholas Nethercote [:njn] 2013-06-16 18:36:03 PDT
Thanks for working on this.  When you post a patch, please make sure you make a "review" or "feedback" request from somebody, otherwise you might not get a response.

mfbt/Util.h serves as a good example for this case.  I think you want something like this:

  #ifndef mozilla_MemoryReporting_h_
  #define mozilla_MemoryReporting_h_

  #ifdef __cplusplus

  namespace mozilla {
   
  // This is for functions that are like malloc_usable_size. Such functions are
  // used for measuring the size of data structures.
  typedef size_t(*MallocSizeOf)(const void *p);
   
  } // namespace mozilla

  #endif // __cpluscplus

  // This is an alternative to mozilla::MallocSizeOf for C code.
  typedef size_t(*MozMallocSizeOf)(const void *p);
   
  #endif // mozilla_MemoryReporting_h_

(mfbt/STYLE suggests the |Moz| prefix for C types.)

It makes me sad that the Mozilla namespace is |mozilla| and not |moz| :(

I think it's fine to have |#include "MemoryReporting.h"| in both Foo.h
and Foo.cpp, so long as |MallocSizeOf| occurs in both.  Compilers (GCC and
clang, at least) understand the #ifndef wrapper in MemoryReporting.h and can
avoid reading it more than once.

Finally, in C++ header files you'll need to use |mozilla::MallocSizeOf|.  But many .cpp files will have |using namespace mozilla;| in them, in which case you only need to write |MallocSizeOf|.  But some .cpp files lack the |using| statement.  So I'd adjust your script to omit the namespace qualification in .cpp files, and see how many compile failures you get, and then fix those ones by hand -- either by qualifying, or by adding the |using| statement.
Comment 12 Nicholas Nethercote [:njn] 2013-06-16 18:55:01 PDT
Oh, and since MemoryReporting.h will be used in C code, it should only use C-style comments, not C++-style comments.  Thanks.
Comment 13 Catalin Iacob 2013-06-20 14:21:49 PDT
Created attachment 765600 [details] [diff] [review]
Part 1
Comment 14 Catalin Iacob 2013-06-20 14:23:10 PDT
Created attachment 765601 [details] [diff] [review]
Part 2
Comment 15 Catalin Iacob 2013-06-20 14:24:53 PDT
Created attachment 765603 [details] [diff] [review]
Part 3
Comment 16 Catalin Iacob 2013-06-20 14:26:29 PDT
Created attachment 765604 [details] [diff] [review]
Part 4
Comment 17 Catalin Iacob 2013-06-20 14:27:21 PDT
Created attachment 765606 [details] [diff] [review]
Part 5
Comment 18 Catalin Iacob 2013-06-20 14:34:26 PDT
Attached current patches. Parts 4 and 5 are quite big, I generated them with a very hackish script and manually fixed 3 or 4 files. The script uses mozilla:: only if there is no using namespace mozilla in that file. It always includes MemoryReporting.h and it tries to put the include in a good place (near the other mozilla/.... includes, alphabetically, or as the first include if there isn't a mozilla/... include already).

The current script version is at https://github.com/cataliniacob/misc/blob/master/rename_moz_type.py

They were written against bb7c53fbb8cd from June 15 and rebased before uploading them, even in just 5 days I had to remove 4 more nsMallocSizeOfFun that trickled in.

This definitely needs a try run to check that it builds everywhere; I hope it does but you never know.
Comment 19 Nicholas Nethercote [:njn] 2013-06-20 17:55:32 PDT
Comment on attachment 765600 [details] [diff] [review]
Part 1

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

::: mfbt/MemoryReporting.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

mfbt/STYLE doesn't mention modelines, so I'd follow https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style and change it to this:

  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
  /* This Source Code Form is subject to the terms of the Mozilla Public
   * License, v. 2.0. If a copy of the MPL was not distributed with this
   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
Comment 20 Nicholas Nethercote [:njn] 2013-06-20 17:57:40 PDT
Comment on attachment 765601 [details] [diff] [review]
Part 2

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

Do you have commit access?  If not, I can try-server and land these patches for you.
Comment 21 Nicholas Nethercote [:njn] 2013-06-20 18:12:23 PDT
Comment on attachment 765604 [details] [diff] [review]
Part 4

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

jsdhash.{h,cpp} were just removed in bug 884649.

https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering describes the SpiderMonkey #include ordering rules, which explains why I've asked you to add blank lines in various places.  (You might notice that not all the files currently follow these rules... it's because the rules are quite new and I'm in the process of fixing them.)

::: js/public/MemoryMetrics.h
@@ +9,5 @@
>  
>  // These declarations are not within jsapi.h because they are highly likely to
>  // change in the future. Depend on them at your own risk.
>  
> +#include "mozilla/MemoryReporting.h"

JS style says to put a blank line after this, before the <string.h>

::: js/public/Utility.h
@@ +9,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Compiler.h"
> +#include "mozilla/MemoryReporting.h"

You don't need this.

::: js/src/ion/BaselineJIT.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/ion/BaselineJIT.h
@@ +8,5 @@
>  #define jsion_baseline_jit_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Blank line between this line and the jscntxt.h line.

::: js/src/ion/Ion.cpp
@@ +3,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/ion/Ion.h
@@ +8,5 @@
>  #define jsion_ion_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Again, blank line after.

::: js/src/ion/IonCompartment.h
@@ +8,5 @@
>  #define jsion_ion_compartment_h__
>  
>  #ifdef JS_ION
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/jsmath.h
@@ +6,5 @@
>  
>  #ifndef jsmath_h___
>  #define jsmath_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/jsobj.h
@@ +14,5 @@
>   * ordered property names, called the map; and a dense vector of property
>   * values, called slots.  The map/slot pointer pair is GC'ed, while the map
>   * is reference counted and the slot vector is malloc'ed.
>   */
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/ArgumentsObject-inl.h
@@ +7,5 @@
>  #ifndef ArgumentsObject_inl_h___
>  #define ArgumentsObject_inl_h___
>  
>  #include "vm/ArgumentsObject.h"
> +#include "mozilla/MemoryReporting.h"

JS style is to put a blank line between vm/ArgumentsObject.h and what comes afterwards.

::: js/src/vm/ArgumentsObject.h
@@ +6,5 @@
>  
>  #ifndef ArgumentsObject_h___
>  #define ArgumentsObject_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/RegExpObject.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "vm/RegExpObject.h"
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/src/vm/RegExpStatics-inl.h
@@ +6,5 @@
>  
>  #ifndef RegExpStatics_inl_h__
>  #define RegExpStatics_inl_h__
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.

::: js/xpconnect/src/XPCMaps.h
@@ +8,5 @@
>  
>  #ifndef xpcmaps_h___
>  #define xpcmaps_h___
>  
> +#include "mozilla/MemoryReporting.h"

Blank line afterwards.
Comment 22 Nicholas Nethercote [:njn] 2013-06-20 18:26:10 PDT
Comment on attachment 765606 [details] [diff] [review]
Part 5

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

This touched a lot more files than I expected! :)  Thanks for doing it!

And thanks for going to the effort of putting the #include lines in order with other |#include "mozilla/*.h"| lines.

If you need me to land the patches, I'll do that once you've updated them for the review comments.  It's Friday here, so depending on timing I might not get to it until Monday next week.

::: xpcom/string/src/nsSubstring.cpp
@@ +8,5 @@
>  #define ENABLE_STRING_STATS
>  #endif
>  
>  #ifdef ENABLE_STRING_STATS
> +#include "mozilla/MemoryReporting.h"

This is in the wrong place -- please move it out of the ENABLE_STRING_STATS block.

(I may have missed other similar cases;  it would be worth doing a search for them if you can.)
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2013-06-20 18:41:56 PDT
Comment on attachment 765600 [details] [diff] [review]
Part 1

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

Thanks for the patch!  Do you want to post a new one with the tweaks, then one of us can push it?

::: mfbt/MemoryReporting.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Yeah, we seem to be all over the map as far as mode lines go -- probably because not everyone cares about them, so they're only around inconsistently.  That'll do as well as anything til they all get cleaned up, I guess.

@@ +17,5 @@
> +/*
> + * This is for functions that are like malloc_usable_size.  Such functions are
> + * used for measuring the size of data structures.
> + */
> +typedef size_t(*MallocSizeOf)(const void *p);

There's truly no good way to format function pointer typedefs, but I think I can reasonably take a stand against mashing the return type in the signature up against the parentheses surrounding the type being defined.  :-)  It just makes the whole conjoined mess even more conjoined and more messy.  Also, * goes by the type per mfbt/STYLE.  So then you'd have:

typedef size_t (*MallocSizeOf)(const void* p);

And further down:

typedef size_t (*MozMallocSizeOf)(const void* p);
Comment 24 Catalin Iacob 2013-06-23 04:44:24 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> Yeah, we seem to be all over the map as far as mode lines go -- probably
> because not everyone cares about them, so they're only around
> inconsistently.  That'll do as well as anything til they all get cleaned up,
> I guess.

I updated this to Nick's suggestion from comment 19. If there is a documented standard it's better to follow it.

> typedef size_t (*MallocSizeOf)(const void* p);
> 
> And further down:
> 
> typedef size_t (*MozMallocSizeOf)(const void* p);

Updated, thanks. Should have read mfbt/STYLE more carefully, I only did a cursory read of it.
Comment 25 Catalin Iacob 2013-06-23 04:48:49 PDT
(In reply to Nicholas Nethercote [:njn] from comment #21)
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.
> 23include_ordering describes the SpiderMonkey #include ordering rules, which
> explains why I've asked you to add blank lines in various places.

I updated all files in your list to add the blank lines and found another one where it was missing. In the mean time a new usage of JSMallocSizeOfFun popped up, I converted that as well.

> ::: js/public/Utility.h
> @@ +9,5 @@
> >  
> >  #include "mozilla/Assertions.h"
> >  #include "mozilla/Attributes.h"
> >  #include "mozilla/Compiler.h"
> > +#include "mozilla/MemoryReporting.h"
> 
> You don't need this.

True, removed.
Comment 26 Catalin Iacob 2013-06-23 04:53:31 PDT
(In reply to Nicholas Nethercote [:njn] from comment #22)
> ::: xpcom/string/src/nsSubstring.cpp
> @@ +8,5 @@
> >  #define ENABLE_STRING_STATS
> >  #endif
> >  
> >  #ifdef ENABLE_STRING_STATS
> > +#include "mozilla/MemoryReporting.h"
> 
> This is in the wrong place -- please move it out of the ENABLE_STRING_STATS
> block.
> 
> (I may have missed other similar cases;  it would be worth doing a search
> for them if you can.)

Good catch. I searched for #ifdef in the whole patch and found another one like this in gfx/thebes/gfxFT2FontList.h in an #ifdef XP_WIN. Came with the automated adding of the header.
Comment 27 Catalin Iacob 2013-06-23 05:05:49 PDT
Created attachment 766417 [details] [diff] [review]
Part 1
Comment 28 Catalin Iacob 2013-06-23 05:06:22 PDT
Created attachment 766418 [details] [diff] [review]
Part 2
Comment 29 Catalin Iacob 2013-06-23 05:06:50 PDT
Created attachment 766419 [details] [diff] [review]
Part 3
Comment 30 Catalin Iacob 2013-06-23 05:07:40 PDT
Created attachment 766420 [details] [diff] [review]
Part 4
Comment 31 Catalin Iacob 2013-06-23 05:10:54 PDT
Created attachment 766422 [details] [diff] [review]
Part 5

Besides addressing the comments, this has a few more whitespace changes in gfx/thebes/gfxPlatformFontList.cpp and gfx/thebes/gfxPlatformFontList.h to align parameter names.
Comment 32 Catalin Iacob 2013-06-23 05:14:46 PDT
Added new patches, these should address all comments and they're rebased on top of current mozilla-central.

I have neither try nor commit access. I'll ask on IRC for somebody to push these to try, so hopefully Monday morning somebody finds a green try run and lands these; the patches get stale quite quickly because others reintroduce uses of the replaced typedefs.
Comment 33 Catalin Iacob 2013-06-23 05:31:27 PDT
Try run at https://tbpl.mozilla.org/?tree=Try&rev=235430b87bc7
Comment 34 Nicholas Nethercote [:njn] 2013-06-24 15:53:34 PDT
That try run was green.  I'll do another try run (including debug builds, which are worth doing, in my experience) and then land.  Thanks.
Comment 36 neil@parkwaycc.co.uk 2013-06-25 01:10:36 PDT
Comment on attachment 766422 [details] [diff] [review]
Part 5

>diff --git a/xpcom/glue/nsCOMArray.cpp b/xpcom/glue/nsCOMArray.cpp
>--- a/xpcom/glue/nsCOMArray.cpp
>+++ b/xpcom/glue/nsCOMArray.cpp
>@@ -1,13 +1,14 @@
> /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+#include "mozilla/MemoryReporting.h"
> #include "nsCOMArray.h"
What's correct style here? nsCOMArray.h has to include MemoryReporting.h to declare the function defined here.
Comment 37 :Ms2ger (⌚ UTC+1/+2) 2013-06-25 06:20:12 PDT
Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.
Comment 38 Nathan Froyd [:froydnj] 2013-06-25 06:22:41 PDT
(In reply to :Ms2ger from comment #37)
> Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.

I think the point was more that nsCOMArray.h already includes MemoryReporting.h.
Comment 39 Catalin Iacob 2013-06-25 10:13:06 PDT
Regarding having the include in the .cpp file and in the corresponding .h file, I think it's ok style wise if both files use the typedef, as it makes the files more self contained. The .cpp could be the only one using the typedef now or later. I find it weird that removing the include from the .h would require adding it to the .cpp. I realize in practice .cpp files end up relying on headers included only by their headers but I see that as a downside not something to strive for. I did ask about this in comment 10 and Nick agreed in comment 11.

The positions of the includes for Part 5 are decided by the automated script that generated them: together with the other mozilla/... includes if they exist or as the first header if not. I chose this as mfbt/STYLE says mfbt headers should be included first and I didn't realize that the corresponding header is special and should go on top.

I did manually fix js/ in Part 4 to have the corresponding .h first. Part 5 is considerably bigger though, and the include order in the files it touches is messier than in js/ to start with (for example mfbt includes aren't always first), so I didn't do it there. I can do that now if you think that's a good idea.
Comment 41 Nicholas Nethercote [:njn] 2013-06-25 15:42:55 PDT
> Correct style is that nsCOMArray.h is the first include in nsCOMArray.cpp.

True.  I forgot about that, sorry.

As for the #inclusion into both the .h and .cpp files, it doesn't worry me.  If you take the view that you shouldn't #include anything indirectly that you haven't #included directly, it's the right thing to do.
Comment 42 Catalin Iacob 2013-06-30 07:39:31 PDT
Created attachment 769492 [details] [diff] [review]
Followup, don't include MemoryReporting.h before a .cpp's own header

Finally had some time to address the concerns in comment 37 and comment 41.

This patch was generated by another hackish script, at https://github.com/cataliniacob/misc/blob/master/own_header_first.py

The script basically searches if the "own include comes first" was respected and the MemoryReporting.h include broke it.

In other words, if "MemoryReporting" is now the first include and the next include afterwards is the header's own include then it swaps them and adds newlines as appropriate to separate the include sections.
Comment 43 Nicholas Nethercote [:njn] 2013-06-30 18:13:36 PDT
Comment on attachment 769492 [details] [diff] [review]
Followup, don't include MemoryReporting.h before a .cpp's own header

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

Thanks for doing this.  I like how you put a blank line after the module #include, to make it stand out.  I'll land this patch today.
Comment 44 Nicholas Nethercote [:njn] 2013-06-30 18:26:55 PDT
Comment on attachment 766417 [details] [diff] [review]
Part 1

A minor thing:  when posting updates versions of r+'d patches, it's customary to r+ (yourself) the new versions.  That way it's clearer that the final versions have r+.  I'll do this for these patches now.
Comment 45 Nicholas Nethercote [:njn] 2013-06-30 18:58:43 PDT
Part 6 (follow-up):
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f0ca9de75f
Comment 46 Ryan VanderMeulen [:RyanVM] 2013-07-01 05:21:32 PDT
https://hg.mozilla.org/mozilla-central/rev/23f0ca9de75f

Note You need to log in before you can comment on or make changes to this bug.