Make .cpp files in editor/libeditor include their own .h file first

RESOLVED FIXED in mozilla37

Status

()

Core
Editor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla37
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
We have a general best-practice that Foo.cpp's first #include should be for Foo.h (if Foo.h exists).

This lets us verify that Foo.h correctly forward-declares all the types that it uses.  (If it doesn't, then Foo.cpp will fail to compile.

Filing this bug on fixing up .cpp files in editor/libeditor to conform to this.
(Assignee)

Comment 1

4 years ago
Created attachment 8516072 [details] [diff] [review]
part 1: drop unneeded stdio.h includes from 3 files

First: when bumping #includes up for this bug, I noticed that several of these files start with...
> #include <stdio.h>                      // for printf
...and yet don't use printf at all.

So, this patch drops those includes.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8516072 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 2

4 years ago
Created attachment 8516074 [details] [diff] [review]
part 2: add missing forward-decls / #includes

Unsurprisingly, the "main" patch here (which I'll post next) caused build failures, because it caught some missing forward-decls / includes in the .h files.

This patch adds those missing forward-decls / includes, so that we can continue compiling.

(Note: I built with "ac_add_options --disable-unified-compilation" to catch these, since otherwise, unified compilation lets us cheat & have the things we need already declared/defined by previous .cpp files.)
Attachment #8516074 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8516074 [details] [diff] [review]
part 2: add missing forward-decls / #includes

>+++ b/editor/libeditor/nsHTMLEditUtils.h
> #ifndef nsHTMLEditUtils_h__
> #define nsHTMLEditUtils_h__
> 
>+#include "mozilla/Types.h"
> 

(In case it's not clear, this ^ include is to give us "int32_t", which this header uses in some function-signatures lower down.)
(Assignee)

Comment 4

4 years ago
Created attachment 8516075 [details] [diff] [review]
part 3: make foo.cpp include foo.h first

...and here's the main patch here -- making each .cpp file #include its own .h file first.
Attachment #8516075 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 5

4 years ago
(FWIW, I looked for candidate .cpp files to fix here with the following shell command...

for cppfile in *cpp; do hfile=`echo $cppfile | sed "s/.cpp/.h/"`; echo; echo "***$cppfile***"; grep -B4 $hfile $cppfile; done

...and I skimmed the output for chunks with other #includes in grep's before-context from "-B4".)
(Assignee)

Comment 7

4 years ago
(Try run is green.)

Updated

4 years ago
Attachment #8516072 - Flags: review?(ehsan.akhgari) → review+

Updated

4 years ago
Attachment #8516074 - Flags: review?(ehsan.akhgari) → review+

Comment 8

4 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8516074 [details] [diff] [review]
> part 2: add missing forward-decls / #includes
> 
> >+++ b/editor/libeditor/nsHTMLEditUtils.h
> > #ifndef nsHTMLEditUtils_h__
> > #define nsHTMLEditUtils_h__
> > 
> >+#include "mozilla/Types.h"
> > 
> 
> (In case it's not clear, this ^ include is to give us "int32_t", which this
> header uses in some function-signatures lower down.)

You could just include <stdint.h> for that, right?

Updated

4 years ago
Attachment #8516075 - Flags: review?(ehsan.akhgari) → review+
(Assignee)

Comment 9

4 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> You could just include <stdint.h> for that, right?

Hmm, looks like it. Not sure why I'd thought I needed mfbt's Types.h for that. I'll sanity-check that w/ Try, and then land if all is well. Thanks for the review!
(Assignee)

Updated

4 years ago
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/3361018e53e5
https://hg.mozilla.org/mozilla-central/rev/f263abe1e116
https://hg.mozilla.org/mozilla-central/rev/debcb0ca1fec
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.