Closed Bug 1156632 Opened 9 years ago Closed 9 years ago

Remove unused forward class declarations

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(7 files)

I'm going to do this also for any other part of the tree.
Of course I'm not doing it manually.
Attachment #8595162 - Flags: review?(ehsan)
Summary: Remove unused forward declarations in dom/base → Remove unused forward class declarations in dom/base
Summary: Remove unused forward class declarations in dom/base → Remove unused forward class declarations
Attachment #8595162 - Attachment description: fix1.patch → patch 1 - dom/base
Attachment #8595170 - Attachment description: dom/media dom/indexedDB dom/svg → patch 2 - dom/media dom/indexedDB dom/svg
Attachment #8595189 - Flags: review?(ehsan)
Attachment #8595289 - Flags: review?(ehsan)
Attached patch patch 7 - JSSplinter Review
I guess we need a JS peer for this patch.
Attachment #8595291 - Flags: review?(ehsan)
Testing if these patches break something:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=714c52566be0
Comment on attachment 8595291 [details] [diff] [review]
patch 7 - JS

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

Looks good to me. I don't know what all of these were, but it's kind of interesting seeing the code evolution reflected in these.
Attachment #8595291 - Flags: review?(ehsan) → review+
Attachment #8595162 - Flags: review?(ehsan) → review+
Attachment #8595170 - Flags: review?(ehsan) → review+
Attachment #8595175 - Flags: review?(ehsan) → review+
Attachment #8595189 - Flags: review?(ehsan) → review+
Attachment #8595231 - Flags: review?(ehsan) → review+
Attachment #8595289 - Flags: review?(ehsan) → review+
Just so you know, I expect that landing this patch will severely break people's ability to do non-unified builds.  I think there are some people who care about those builds...  Maybe dholbert knows?

Anyways, I'm fine with landing this.  Please make sure other people are as well.
Flags: needinfo?(dholbert)
I don't know who (if anyone) depends on non-unified builds.  IIRC they're officially unsupported,  and probably don't compile at the moment.  And if these patches do what I think they do*, I'm fine with taking them as well.

*(From a quick skim, I think you're just removing forward-decls _in cases where the declared type isn't used at all in that file_, correct?  One alternate thing that I thought you might've been doing was: removing forward-declares that end up being redundant as a result of unified builds -- e.g. if A.cpp and B.cpp both forward-declare & use something, you might be able to remove the forward-declare from B.cpp (or from some headers used only in B.cpp) and still be fine building.  That would be bad, though, because it'd add a fragile dependency, which would be exposed once we added new source files between A.cpp and B.cpp & reshuffled the unified groupings. So, I'd object to cleanup of that sort; but I don't think that's what you're doing here.)
Flags: needinfo?(dholbert)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.