Remove mozilla::IndexSequence and use std::index_sequence instead

RESOLVED FIXED in Firefox 61

Status

()

RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: botond, Assigned: tomabann, Mentored)

Tracking

Trunk
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(3 attachments, 23 obsolete attachments)

23.21 KB, text/plain
Details
23.06 KB, text/plain
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
(Reporter)

Description

a year ago
In bug 1163329, we added a utility called mozilla::IndexSequence [1] for expanding a tuple into a variadic argument list. It was analogous to std::index_sequence from the C++11 standard library, which we couldn't use at the time.

Since we can now use the C++11 standard library, we should remove mozilla::IndexSequence and just use std::index_sequence instead.

[1] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/mfbt/IndexSequence.h
If we're going that way, should we consider removing more of the mfbt stuff that's an exact duplicate of c++ std? E.g., Move, Forward, type traits, etc.
(Reporter)

Comment 2

a year ago
If their semantics match, and all our supported compilers swallow the std versions, I don't see why not.
Though of course it would be a massive change (all the #includes, de-capitalizing words...), so I'm not volunteering! (Yet) :-P
Or could it be done with some scripting and/or clang-tidying?
(Reporter)

Comment 4

a year ago
(In reply to Gerald Squelart [:gerald] from comment #3)
> Or could it be done with some scripting and/or clang-tidying?

For utilities with many uses where the APIs match exactly, scripting and/or clang-tidying may be a good time.

For cases like this, where there are only a handful of uses and the APIs don't match exactly (std::index_sequence_for is a template alias but we had to make mozilla::IndexSequenceFor a regular metafunction because a template alias crashed MSVC 2013), I think doing it in bits and pieces like this works best. And the bits and pieces make good mentored bugs :)
(In reply to Gerald Squelart [:gerald] from comment #3)
> Though of course it would be a massive change (all the #includes,
> de-capitalizing words...), so I'm not volunteering! (Yet) :-P
> Or could it be done with some scripting and/or clang-tidying?

An easy first step could be to change the mfbt names to be typedefs for the std names. We would use the std implementations and remove custom mfbt code without disrupting all the call sites.
(In reply to Chris Peterson [:cpeterson] from comment #5)
> An easy first step could be to change the mfbt names to be typedefs for the
> std names. We would use the std implementations and remove custom mfbt code
> without disrupting all the call sites.

I doubt we can do that: Functions like Move can't be typedef'd, and our types' inner types contain different capitalizations too, e.g.: mozilla::RemoveConst<T>::Type vs std::remove_const<T>::type.


Anyway, maybe we should discuss this elsewhere (new bug / m.d.platform?), and keep this bug here for the smaller IndexSequence change. Sorry for hijacking your bug, Botond!
We can remove more stuff, just have to be careful that we're not depending on/gaining value from interpolations our stuff permits us to do.  For example, mozilla::UniquePtr has a bunch of interpolation of our own assertion goo that std::unique_ptr does not have (and unless various STLs start adding ways to inject code into the standard ones, cannot).  Given those extra bits, I'm not sure that we're likely to move to the standard one any time soon at all.

Also, as regards Move.h, there's a bunch of documentation in there that I would like to preserve *somewhere*.  Move.h is a fine location for this, when we're using our own primitives.  When we're not using our own primitives, there's not an obvious location for this, that developers will naturally find when trying to figure out how to use stuff, or how to debug their own uses.  I don't know what the answer is here.

std::index_sequence is a particularly good place to replace, tho -- there is no special functionality we're adding here, and compiler intrinsics let the compiler implement this sort of thing much more efficiently than we can.  https://ldionne.com/2015/11/29/efficient-parameter-pack-indexing/  https://reviews.llvm.org/rL252036
(Reporter)

Comment 8

11 months ago
Just to clarify, for any potential contributors looking to take this bug: it's available to be taken. The discussion in comment 1 through comment 7 is a meta-discussion and can be ignored for your purposes.

Comment 9

10 months ago
As I am a complete newbie to this can you pls tell me where I need to make the changes(remove mozilla::IndexSequence to std::index_sequence )how can i open this code to edit it?
Flags: needinfo?(botond)
(Reporter)

Comment 10

10 months ago
(In reply to pranjal2k16 from comment #9)
> As I am a complete newbie to this can you pls tell me where I need to make
> the changes(remove mozilla::IndexSequence to std::index_sequence )how can i
> open this code to edit it?

Thanks for your interest!

The first step is to check out and build the Firefox source code, as described here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Let me know what you've done that, and I'll give some more guidance about the changes we need to make in this bug.
Flags: needinfo?(botond)
(Reporter)

Comment 11

10 months ago
Were you able to build Firefox using the steps I linked to? Let me know if you ran into any issues, I'm happy to try and help.
Flags: needinfo?(pranjal2k16)

Comment 12

10 months ago
(In reply to Botond Ballo [:botond] from comment #11)
> Were you able to build Firefox using the steps I linked to? Let me know if
> you ran into any issues, I'm happy to try and help.

My ubuntu bootloader got corrupted so I had to re-install it and now I will again build firefox.Sorry for the late reply.
Flags: needinfo?(pranjal2k16)
(Assignee)

Comment 13

10 months ago
Hi Botond,

Is this bug available for me to work on?

I am completely new to this and this bug seems like a good starting point.
Flags: needinfo?(botond)
(Reporter)

Comment 14

10 months ago
Hi Tom! 

Thanks for your interest. Yes, the bug is available.

The first thing to do is to build Firefox using the instructions linked from comment 10. Please give that a try, and post back once you're done (or if you run into any issues).
Flags: needinfo?(botond)
(Assignee)

Comment 15

10 months ago
Thanks for the quick response Botond,

I've already followed those steps and built Firefox and there doesn't seem to be any problems with it.
(Reporter)

Comment 16

10 months ago
Good stuff! I've assigned the bug to you.

As background, if you're not already familiar with std::index_sequence, std::make_index_sequence, and std::index_sequence_for, have a look at their documentation here [1].

We have a header, mfbt/IndexSequence.h [2], which defines our own versions of these utilities, called mozilla::IndexSequence, mozilla::MakeIndexSequence, and mozilla::IndexSequenceFor, which we'd like to replace with the standard ones.

You can use our Searchfox code search tool to find usages of these three utilities in our codebase [3] [4] [5].

The work in this bug involves going through the usages and changing them to their standard equivalents, and then removing the mfbt/IndexSequence.h header.

One small wrinkle is that std::make_index_sequence and std::index_sequence_for are type aliases, used simply like this:

  std::index_sequence_for<Args...>

while the mozilla versions are "metafunctions", used in this more verbose way:

  typename mozilla::IndexSequenceFor<Args...>::Type

(This is for historical reasons; we tried to make them type aliases but that caused the version of MSVC we used at the time to crash). Be sure to take this difference into account when switching from one to the other.

That should be enough to get you started. Please try making the described changes, check that the modified code still complies (by running |mach build| again), and then post the resulting patch for review.

If you run into any issues, feel free to ask questions here!

[1] http://en.cppreference.com/w/cpp/utility/integer_sequence
[2] https://searchfox.org/mozilla-central/source/mfbt/IndexSequence.h
[3] https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3AIndexSequence&redirect=false
[4] https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3AMakeIndexSequence&redirect=false
[5] https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3AIndexSequenceFor&redirect=false
Assignee: nobody → tomabann
(Assignee)

Comment 17

10 months ago
Created attachment 8965291 [details] [diff] [review]
Removed mfbt/IndexSequence.h
(Assignee)

Comment 18

10 months ago
Hi Botond, 

Please disregard the patch I just sent, I tried using bzexport in order to submit the patch and ended up only posting the latest commit. 

The changes have been applied and it compiles successfully on my machine. I am however unsure of what to do with creating and applying the patch. 

Should I simply push all my changes to MozReview or do you want me to post here, and if i should post them here what would be the best way to do so with all the commits i have made?
Flags: needinfo?(botond)
(Reporter)

Comment 19

10 months ago
(In reply to Tom Bannister from comment #18)
> Should I simply push all my changes to MozReview or do you want me to post
> here, and if i should post them here what would be the best way to do so
> with all the commits i have made?

You can feel free to either use MozReview, or post the patches here as attachments. MozReview takes more time to set up initially, but pays off in the long run if you're going to be making multiple contributions over time.
Flags: needinfo?(botond)
(Reporter)

Comment 20

10 months ago
(In reply to Botond Ballo [:botond] from comment #19)
> MozReview takes more time to set up initially, but pays off in
> the long run if you're going to be making multiple contributions over time.

(I guess for completeness I should also mention that MozReview is going to be replaced with a new review system in the near future, so there may not be that much of a "long run" for it. But that shouldn't stop you from setting it up if you'd like.)
(Reporter)

Comment 21

10 months ago
(In reply to Botond Ballo [:botond] from comment #19)
> or post the patches here as attachments

Oh, and if you want to go with this approach and are wondering how to create the files to attach, I suggest:

  hg export -o <filename> <revision>

(You can also automate the process with 'bzexport', as it seems you've tried, but I'm not familiar with that so you'd have to figure out how to use it yourself.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

10 months ago
Hi Botond, 

All the patches have been posted, please let me know if there is anything i have to change :)
Flags: needinfo?(botond)
(Reporter)

Comment 42

10 months ago
I see now why you were asking about how to post the patches - there are rather many of them :)

While we generally encourage splitting up patches into logical chunks, in a case like this where you're making the same kind of change to multiple files, it would have been appropriate to put all the changes into a single patch.

Since you've already split the patches like this, I'll leave it up to you whether you want to leave them split, or combine them into one. However, even if you choose to leave them split, these two patches should be combined into one:

  Bug 1434710 - Replaced mozilla::MakeIndexSequence with std::make_index_sequence in xpcom/string/nsASCIIMask.h. 
  Bug 1434710 - Removed ::Type{} from std::make_index_sequence<128>::Type{} in xpcom/string/nsASCIIMask.h.

That's because the intermediate state, std::make_index_sequence<128>::Type{}, does not compile, and we prefer that each patch in a patch series compiles (otherwise e.g. bisections can break).

You can combine patches in a patch series with "hg histedit".
Flags: needinfo?(botond)
(Reporter)

Comment 43

10 months ago
mozreview-review
Comment on attachment 8965600 [details]
Bug 1434710 - Replaced typename IndexSequenceFor<Args...>::Type() with std::index_sequence_for<Args...> in ipc/chromium/src/base/task.h.

https://reviewboard.mozilla.org/r/234432/#review240234

::: ipc/chromium/src/base/task.h:47
(Diff revision 1)
>  // from the given tuple.
>  template<class ObjT, class Method, typename... Args>
>  void DispatchTupleToMethod(ObjT* obj, Method method, mozilla::Tuple<Args...>& arg)
>  {
> -  details::CallMethod(typename mozilla::IndexSequenceFor<Args...>::Type(),
> +  details::CallMethod(std::index_sequence_for<Args...>,
>                        obj, method, arg);

Might as well combine the two lines into one.

::: ipc/chromium/src/base/task.h:55
(Diff revision 1)
>  // Same as above, but call a function.
>  template<typename Function, typename... Args>
>  void DispatchTupleToFunction(Function function, mozilla::Tuple<Args...>& arg)
>  {
> -  details::CallFunction(typename mozilla::IndexSequenceFor<Args...>::Type(),
> +  details::CallFunction(std::index_sequence_for<Args...>,
>                          function, arg);

Likewise.
(Reporter)

Comment 44

10 months ago
The patches generally look good; the one general comment I have is that, since std::index_sequence, std::make_index_sequence, and std::index_sequence_for are defined in the header <utility>, we should include that header in the files where we use these facilities.
(Reporter)

Comment 45

10 months ago
(In reply to Botond Ballo [:botond] from comment #42)
> Since you've already split the patches like this, I'll leave it up to you
> whether you want to leave them split, or combine them into one.

FWIW, combining them into one would be less work for both of us :)
(Reporter)

Comment 46

10 months ago
mozreview-review
Comment on attachment 8965605 [details]
Bug 1434710 - Removed mfbt/IndexSequence.h.

https://reviewboard.mozilla.org/r/234442/#review240246

As we're removing a file, we need to remove it from the build system metadata as well. That involves removing this line:

https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/mfbt/moz.build#47
Attachment #8965605 - Flags: review?(botond)
(Reporter)

Comment 47

10 months ago
(In reply to Botond Ballo [:botond] from comment #16)
> Please try making the described
> changes, check that the modified code still complies (by running |mach
> build| again), and then post the resulting patch for review.

It seems to me like you skipped the "check that the modified code still complies (by running |mach build| again)" part, because the patch series you posted contains several compiler errors.

Please try compiling the patches, fix the errors, and then post updated patches. (If you need help fixing a particular compiler error, feel free to post the error here.)
(Reporter)

Comment 48

10 months ago
(I'm not going bother manually clearing all the review flags, but consider my first round of review to be complete, with the feedback contained in comments 42-47).
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8965587 - Attachment is obsolete: true
Attachment #8965587 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965588 - Attachment is obsolete: true
Attachment #8965588 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965589 - Attachment is obsolete: true
Attachment #8965589 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965590 - Attachment is obsolete: true
Attachment #8965590 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965591 - Attachment is obsolete: true
Attachment #8965591 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965592 - Attachment is obsolete: true
Attachment #8965592 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965593 - Attachment is obsolete: true
Attachment #8965593 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965594 - Attachment is obsolete: true
Attachment #8965594 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965596 - Attachment is obsolete: true
Attachment #8965596 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965597 - Attachment is obsolete: true
Attachment #8965597 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965598 - Attachment is obsolete: true
Attachment #8965598 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965599 - Attachment is obsolete: true
Attachment #8965599 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965600 - Attachment is obsolete: true
Attachment #8965600 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965601 - Attachment is obsolete: true
Attachment #8965601 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965602 - Attachment is obsolete: true
Attachment #8965602 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965603 - Attachment is obsolete: true
Attachment #8965603 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965604 - Attachment is obsolete: true
Attachment #8965604 - Flags: review?(botond)
(Assignee)

Updated

10 months ago
Attachment #8965605 - Attachment is obsolete: true
(Assignee)

Comment 50

10 months ago
Thanks for the review Botond,

I've applied all the changes you wanted and combined all the changesets into one.

I did not receive any compiler errors for this patch, please let me know if you receive any compiler errors as there could potentially be something wrong on my end.

Any help will be greatly appreciated.
Flags: needinfo?(botond)
(Reporter)

Comment 51

10 months ago
Thanks for the updated patch!

I'm still seeing compiler errors with it. Here is the first one:

 1:16.15 In file included from /home/botond/dev/mozilla/central/security/sandbox/linux/reporter/SandboxReporter.cpp:20:
 1:16.15 /home/botond/dev/mozilla/central/objdir-desktop-clang/dist/include/nsThreadUtils.h:1168:49: error: expected '(' for function-style cast or type construction
 1:16.15                   std::index_sequence_for<Ts...>))
 1:16.15                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^

If you're not seeing any errors, then something is indeed going wrong. Could you post the output of "hg summary" and "./mach build"? (You can post it as an attachment.)
Flags: needinfo?(botond)
(Assignee)

Comment 52

10 months ago
Created attachment 8965929 [details]
output from both hg summary and ./mach build

Here's the output from hg summary and ./mach build
Flags: needinfo?(botond)
(Reporter)

Comment 53

10 months ago
Interesting, it looks like it's not trying to build the modified files at all. Perhaps you are using an artifact build?

When you were doing the initial build following the instructions linked from comment 10, and you got to the step where you run |./mach bootstrap|, at one point it showed you a choice like this:

  Please choose the version of Firefox you want to build:
  1. Firefox for Desktop Artifact Mode
  2. Firefox for Desktop
  3. Firefox for Android Artifact Mode
  4. Firefox for Android
  Your choice:

Did you by chance pick #1 there?

If so, that's the problem; please run |./mach bootstrap| again, pick #2, and then try |./mach build| again.

("Artifact Mode" is for when you're modifying JavaScript code only; for the compiled-code parts of the browser, it downloads a prebuilt package rather than building it locally. If you modify any C++ or Rust code, you need to use the regular build mode.)
Flags: needinfo?(botond)
(Assignee)

Comment 54

10 months ago
Created attachment 8965930 [details]
machbuildoutput

I did initially choose #1. I reran it and chose #2 this time. When i ran mach build again it did not throw up any compiler errors. I've included the output as an attachment.
Flags: needinfo?(botond)
(Assignee)

Comment 55

10 months ago
Nevermind, i got it working. I deleted the --enable-artifact-builds from my mozconfig file and it is now trying to build the modified files
Flags: needinfo?(botond)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8965595 - Attachment is obsolete: true
Attachment #8965595 - Flags: review?(botond)
(Assignee)

Comment 57

9 months ago
Hi Botond,

I've decided to restart the entire patch so that a lot less compiler errors are displayed when mach build is entered. 

I've uploaded a partial patch fixing just accessible/base/NotificationController.h. Please let me know what you think of this and if there is anything that i should change.
(Reporter)

Comment 58

9 months ago
Sorry for the poor experience around artifact builds; I filed some bugs (bug 1452697, bug 1452690) on the topic.
(Reporter)

Comment 59

9 months ago
mozreview-review
Comment on attachment 8966166 [details]
Bug 1434710 - Replaced all instances of mozilla::IndexSequence, mozilla::MakeIndexSequence and mozilla::IndexSequenceFor with std::index_sequence, std::make_index_sequence and std::index_sequence_for and removed mfbt/IndexSequence.h.

https://reviewboard.mozilla.org/r/234898/#review240662

This looks good, thanks!

In particular, I see you discovered that keeping the () or {} after index_sequence_for<Args...> that constructs an instance of the type, is important :)

(I'm clearing the review flag because we don't want to submit this patch in isolation.)
Attachment #8966166 - Flags: review?(botond)
(Reporter)

Comment 60

9 months ago
(In reply to Tom Bannister from comment #57)
> I've decided to restart the entire patch so that a lot less compiler errors
> are displayed when mach build is entered. 

By the way, a tip for dealing with large amounts of build output is to run:

  ./mach build binaries 2>&1 | less -R

The "2>&1" redirects the build process's standard error to standard output, to ensure both go into the pipe.

"less" is a pager tool that shows you the first page of the output at first, and allows you to scroll through the rest with the arrow keys.

The "-R" flag is to get "less" to interpret the color markers in the build output correctly.
(Reporter)

Comment 61

9 months ago
(In reply to Botond Ballo [:botond] from comment #60)
> By the way, a tip for dealing with large amounts of build output is to run:
> 
>   ./mach build binaries 2>&1 | less -R

Oh, and "mach build binaries" is a slightly quicker way to rebuild when you've only modified C++ code. The above advice can, of course, be applied to a plain "mach build" as well, as in:

  ./mach build 2>&1 | less -R
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 64

9 months ago
Hi Botond,

I've submitted the patches for the bug. Please let me know what you think of them.
Flags: needinfo?(botond)
(Assignee)

Comment 65

9 months ago
I think i might have made a mistake with the second patch file with tracking the changes made to MozPromise.h and MediaEventSource.h. I was not entirely sure with adding the files to the repo. Should i roll back to the first patch and then start tracking those files then remake the second patch?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8966531 - Attachment is obsolete: true
Attachment #8966531 - Flags: review?(botond)
(Assignee)

Comment 68

9 months ago
Hi Botond,

I think I fixed the mistake that I made in the second patch in the previous set of patches I submitted by reverting back to the first changeset, adding a changeset where I started tracking the two untracked files and then changing those files in the third changeset instead of both adding and changing them in the same changeset. 

Could you try out these patches and let me know if any problems are still present.

If problems are still present I'm thinking of completely reinstalling mozilla on my machine so that I can completely redo the second and third patches. If that is the case how can I make a backup of the first changeset I made so that i can apply it to the newly downloaded copy of mozilla and save myself having to change the files all over again?
(Reporter)

Comment 69

9 months ago
mozreview-review
Comment on attachment 8966866 [details]
Bug 1434710 - Started tracking obj-i686-pc-mingw32/dist/include/MediaEventSource.h and obj-i686-pc-mingw32/dist/include/mozilla/MozPromise.h.

https://reviewboard.mozilla.org/r/235544/#review241546

Files in the object directory should never be added to version control.

What's happening here is that our build system copies header files from their locations in the source tree, to the object directory, and they are then included from there.

As a result, if there are compiler errors in the header files, the paths that show up in the error message are the paths of the copies in the object directory. However, we do not want to modify those copies.

The correct thing to do here is to modify the original copies of the headers in the source tree (in this case, that would be dom/media/MediaEventSource.h and xpcom/threads/MozPromise.h). The next time you build the modified files will be copied to the object directory again.
Attachment #8966866 - Flags: review?(botond)
(Reporter)

Comment 70

9 months ago
mozreview-review
Comment on attachment 8966166 [details]
Bug 1434710 - Replaced all instances of mozilla::IndexSequence, mozilla::MakeIndexSequence and mozilla::IndexSequenceFor with std::index_sequence, std::make_index_sequence and std::index_sequence_for and removed mfbt/IndexSequence.h.

https://reviewboard.mozilla.org/r/234898/#review241558

This part looks good.
Attachment #8966166 - Flags: review?(botond)
(Reporter)

Comment 71

9 months ago
mozreview-review
Comment on attachment 8966867 [details]
Bug 1434710 - Removed mfbt/IndexSequence.h.

https://reviewboard.mozilla.org/r/235546/#review241560

The only thing missing here (besides the fact that the changes to the obj-i686-pc-mingw32 files shouldn't be there, as mentioned above) is that the change to MozPromise.h needs to be made to the source copy at xpcom/threads/MozPromise.h.
Attachment #8966867 - Flags: review?(botond)
(Reporter)

Comment 72

9 months ago
(In reply to Tom Bannister from comment #68)
> If problems are still present I'm thinking of completely reinstalling
> mozilla on my machine so that I can completely redo the second and third
> patches. If that is the case how can I make a backup of the first changeset
> I made so that i can apply it to the newly downloaded copy of mozilla and
> save myself having to change the files all over again?

There's no need to start from scratch. You can run "hg histedit", and "drop" the second and third patches. You can then re-add the necessary changes from the third patch, and use "hg amend" to combine the changes into the first patch. That should give you a single patch with all the changes.

Please give that a try and let me know if you run into any issues!
Flags: needinfo?(botond)
(Reporter)

Comment 73

9 months ago
(In reply to Tom Bannister from comment #68)
> how can I make a backup of the first changeset
> I made so that i can apply it to the newly downloaded copy of mozilla and
> save myself having to change the files all over again?

(But to answer your question for future reference, you can export a commit to a patch file using "hg export", and import such a file back into a repository using "hg import".)
(Reporter)

Comment 74

9 months ago
(In reply to Botond Ballo [:botond] from comment #69)
> The correct thing to do here is to modify the original copies of the headers
> in the source tree (in this case, that would be dom/media/MediaEventSource.h
> and xpcom/threads/MozPromise.h).

If you're wondering how to find out what the source path is for a header, you can search the source tree using a command like:

  find . | grep MozPromise.h

or you can ask Searchfox:

  https://searchfox.org/mozilla-central/search?q=MozPromise.h&path=
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8966866 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8966867 - Attachment is obsolete: true
(Assignee)

Comment 76

9 months ago
Hi Botond,

I have published the patch, please let me know what you think
(Reporter)

Comment 77

9 months ago
mozreview-review
Comment on attachment 8966166 [details]
Bug 1434710 - Replaced all instances of mozilla::IndexSequence, mozilla::MakeIndexSequence and mozilla::IndexSequenceFor with std::index_sequence, std::make_index_sequence and std::index_sequence_for and removed mfbt/IndexSequence.h.

https://reviewboard.mozilla.org/r/234898/#review242086

Thanks, looks good to me!
Attachment #8966166 - Flags: review?(botond) → review+
(Reporter)

Comment 78

9 months ago
I pushed the patch to our Try server, to make sure it's building on all platforms and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f79f2c6ffde79695956d8b15bc4becdae25715b
(Reporter)

Updated

9 months ago
Attachment #8965291 - Attachment is obsolete: true
(Reporter)

Comment 79

9 months ago
(In reply to Botond Ballo [:botond] from comment #78)
> I pushed the patch to our Try server, to make sure it's building on all
> platforms and passing tests:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5f79f2c6ffde79695956d8b15bc4becdae25715b

The Try push is indicating that there are a few remaining issues with the patch.


First, a number of builds are failing with these errors:

TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output; diff is above.
TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | Hint: If the problem is that you renamed a header, and many #includes are no longer in alphabetical order, commit your work and then try `check_spidermonkey_style.py --fixup`. You need to commit first because --fixup modifies your files in place.

This is a bit cryptic, but if we look in the full build log, there is more detail:

[task 2018-04-13T01:12:59.539Z] --- check_spidermonkey_style.py expected output
[task 2018-04-13T01:12:59.539Z] +++ check_spidermonkey_style.py actual output
[task 2018-04-13T01:12:59.539Z] @@ -1,5 +1,8 @@
[task 2018-04-13T01:12:59.539Z]  js/src/tests/style/BadIncludes2.h:1: error:
[task 2018-04-13T01:12:59.539Z]      vanilla header includes an inline-header file "tests/style/BadIncludes2-inl.h"
[task 2018-04-13T01:12:59.539Z] +
[task 2018-04-13T01:12:59.539Z] +js/src/frontend/EitherParser.h:22:24: error:
[task 2018-04-13T01:12:59.539Z] +    "frontend/TokenStream.h" should be included after <utility>
[task 2018-04-13T01:12:59.539Z]  
[task 2018-04-13T01:12:59.539Z]  js/src/tests/style/BadIncludes.h:3: error:
[task 2018-04-13T01:12:59.539Z]      the file includes itself

The important part here is:

  js/src/frontend/EitherParser.h:22:24: error: "frontend/TokenStream.h" should be included after <utility>

So what's happening is:

  - There is a style checker that runs on code in the js/ subdirectory.

  - The style checker requires that the <utility> include, which
    this patch is adding to js/src/frontend/EitherParser.h,
    should come before the "frontend/..." includes.

I'm not sure why this style checker doesn't run during local builds (I asked about that in bug 1453854 comment 1), but in any case the fix is straightforward: just add the <utility> include further above as requested.


The second issue is that OS X builds are failing with the error:

  xpcom/string/nsASCIIMask.h:38:10: error: unknown type name 'MOZ_ALWAYS_INLINE'

MOZ_ALWAYS_INLINE is a macro defined in mfbt/Attributes.h. nsASCIIMask.h uses this macro, but does not include Attributes.h directly; instead, it was relying on the fact that IndexSequence.h was including Attributes.h. Now that we removed IndexSequence.h, Attribute.h is no longer being included and we get this error.

The fix here is also straightforward: add '#include "mozilla/Attributes.h"' to xpcom/string/nsASCIIMask.h.

(If you're wondering why this error didn't show up in your local build, it's because of a build system feature we have called "unified builds", that combines several several .cpp files into one before building, to get faster builds. This can hide errors like the above, as the required include can be brought in from another file that was combined with this one. However, the files that are combined varies from platform to platform, leading to errors like this that only show up on one platform.)


Finally, Android builds are failing with the error:

  obj-firefox/dist/include/mozilla/jni/Natives.h:8:10: fatal error: 'mozilla/IndexSequence.h' file not found

This is a header that's only used on Android, that's still including mozilla/IndexSequence.h [1]. Note that the same file also contains usages of mozilla::IndexSequence and mozilla::IndexSequenceFor that need to be updated as well.

[1] https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/widget/android/jni/Natives.h#8
(Reporter)

Comment 80

9 months ago
By the way, Tom, thanks for your patience as we work through these unanticipated issues. More complications have come up in this bug than I expected when I tagged it as a "good first bug", and I appreciate you sticking with it!
So the basic idea is this style checker is enforcing SpiderMonkey's #include ordering rules.

https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style

The salient part is that #includes following different patterns should be arranged in this order

#include "Foo.h" // Foo.cpp must include its header *first*

#include "mozilla/*.h" // all mfbt headers go here

#include <...> // all system headers go here

#include "js*.h" // all jsbool.h-style headers go here -- we're removing these, so few should exist

#include "x/y.h" // js/Value.h, builtin/StringType.h, etc. go here

In the changes to EitherParser.h, you instead have

#include "mozilla/*.h" // a bunch of these

#include "frontend/*.h" // a few of these

#include <utility> // !!! out of order, should be after the mozilla/* headers

At a quick skim, I think that's the only file that requires changing -- put <utility> between mozilla/*.h and */*.h blocks of #includes -- to get things going again.
Comment hidden (mozreview-request)
(Assignee)

Comment 83

9 months ago
Thanks Botond, 

You've been so helpful with all the problems that have popped while I have been working on this.

Hopefully this patch will meet all the testing criteria :)
(Reporter)

Comment 85

9 months ago
(In reply to Botond Ballo [:botond] from comment #84)
> Thanks! Here is a new Try push for the updated patch:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=32e6b671a8c3df0928b5ae4edd0c75808fd8de45

Looking much better! I'll go ahead and submit the patch.

Comment 86

9 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afb4d1027523
Replaced all instances of mozilla::IndexSequence, mozilla::MakeIndexSequence and mozilla::IndexSequenceFor with std::index_sequence, std::make_index_sequence and std::index_sequence_for and removed mfbt/IndexSequence.h. r=botond

Comment 87

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afb4d1027523
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
status-firefox60: affected → ---
(Reporter)

Comment 88

9 months ago
All done here. Thanks for your work here, Tom!

If you're interested in working on another bug and would like some ideas, let me know!
(In reply to Botond Ballo [:botond] from comment #88)
> All done here. Thanks for your work here, Tom!

I'd like to echo this!  This may seem like a small thing, but it's an important step in both making the stuff we use in-tree as efficient as possible (e.g. comment 7) and making things more approachable/comprehensible by using the standard tools wherever possible.  Thank you!
(Assignee)

Comment 90

9 months ago
Hi Botond,

I am interested in tackling another bug again. I am trying to find bug fixes which would be relatively easy for C++ beginners as I have just started learning C++. Any ideas would help tremendously.
Flags: needinfo?(botond)
(Reporter)

Comment 91

9 months ago
(In reply to Tom Bannister from comment #90)
> I am interested in tackling another bug again. I am trying to find bug fixes
> which would be relatively easy for C++ beginners as I have just started
> learning C++. Any ideas would help tremendously.

I don't have any bugs I'm mentoring myself that fit the bill at the moment, but you should be able to find some using the Bugs Ahoy website [1].

In particular, this query [2] turns up some bugs that might work (such as bug 1427765).

(Note, some bugs on the list may have an assignee already, but if there hasn't been activity for a few months, you're welcome to ask to take the bug over.)

[1] https://www.joshmatthews.net/bugsahoy/
[2] https://www.joshmatthews.net/bugsahoy/?cpp=1&simple=1
Flags: needinfo?(botond)
You need to log in before you can comment on or make changes to this bug.