Closed
Bug 1434710
Opened 7 years ago
Closed 7 years ago
Remove mozilla::IndexSequence and use std::index_sequence instead
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: botond, Assigned: tomabann, Mentored)
Details
(Whiteboard: [lang=c++])
Attachments
(3 files, 23 obsolete files)
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•7 years 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•7 years 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 :)
Comment 5•7 years ago
|
||
(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!
Comment 7•7 years ago
|
||
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•7 years ago
|
||
Comment 9•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Assignee | ||
Comment 18•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8965587 -
Attachment is obsolete: true
Attachment #8965587 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965588 -
Attachment is obsolete: true
Attachment #8965588 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965589 -
Attachment is obsolete: true
Attachment #8965589 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965590 -
Attachment is obsolete: true
Attachment #8965590 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965591 -
Attachment is obsolete: true
Attachment #8965591 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965592 -
Attachment is obsolete: true
Attachment #8965592 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965593 -
Attachment is obsolete: true
Attachment #8965593 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965594 -
Attachment is obsolete: true
Attachment #8965594 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965596 -
Attachment is obsolete: true
Attachment #8965596 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965597 -
Attachment is obsolete: true
Attachment #8965597 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965598 -
Attachment is obsolete: true
Attachment #8965598 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965599 -
Attachment is obsolete: true
Attachment #8965599 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965600 -
Attachment is obsolete: true
Attachment #8965600 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965601 -
Attachment is obsolete: true
Attachment #8965601 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965602 -
Attachment is obsolete: true
Attachment #8965602 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965603 -
Attachment is obsolete: true
Attachment #8965603 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965604 -
Attachment is obsolete: true
Attachment #8965604 -
Flags: review?(botond)
Assignee | ||
Updated•7 years ago
|
Attachment #8965605 -
Attachment is obsolete: true
Assignee | ||
Comment 50•7 years 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•7 years 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•7 years ago
|
||
Here's the output from hg summary and ./mach build
Flags: needinfo?(botond)
Reporter | ||
Comment 53•7 years 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•7 years ago
|
||
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•7 years 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•7 years ago
|
Attachment #8965595 -
Attachment is obsolete: true
Attachment #8965595 -
Flags: review?(botond)
Assignee | ||
Comment 57•7 years 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•7 years ago
|
||
Sorry for the poor experience around artifact builds; I filed some bugs (bug 1452697, bug 1452690) on the topic.
Reporter | ||
Comment 59•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8966531 -
Attachment is obsolete: true
Attachment #8966531 -
Flags: review?(botond)
Assignee | ||
Comment 68•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
Attachment #8966866 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8966867 -
Attachment is obsolete: true
Assignee | ||
Comment 76•7 years ago
|
||
Hi Botond,
I have published the patch, please let me know what you think
Reporter | ||
Comment 77•7 years 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•7 years 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•7 years ago
|
Attachment #8965291 -
Attachment is obsolete: true
Reporter | ||
Comment 79•7 years 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•7 years 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!
Comment 81•7 years ago
|
||
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•7 years 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 84•7 years ago
|
||
Thanks! Here is a new Try push for the updated patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32e6b671a8c3df0928b5ae4edd0c75808fd8de45
Reporter | ||
Comment 85•7 years 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•7 years 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•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•7 years ago
|
status-firefox60:
affected → ---
Reporter | ||
Comment 88•7 years 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!
Comment 89•7 years ago
|
||
(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•7 years 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•7 years 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.
Description
•