Closed
Bug 1449094
Opened 7 years ago
Closed 7 years ago
Implement constexpr mozilla::AllOf
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: emk, Assigned: emk)
References
Details
Attachments
(1 file)
Until C++20 constexpr all_of is available everywhere.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
![]() |
||
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237150 This patch appears to be missing Algorithm.h?
Attachment #8962745 -
Flags: review?(nfroyd)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237150 Oops, I forgot `hg add`.
![]() |
||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237180 No real objections, but I would like to have a look at the documentation before this lands. ::: mfbt/Algorithm.h:7 (Diff revision 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/. */ > + > +#ifndef mozilla_Algorithm_h Please add a one-line comment summarizing the purpose of this file (I guess a polyfill for `<algorithm>`)? ::: mfbt/Algorithm.h:14 (Diff revision 2) > +template <class Iter, class Pred> > +constexpr bool AllOf(Iter aFirst, Iter aLast, Pred p) Do uses of this in the baseline compiler GCC builds cause errors? ::: mfbt/Algorithm.h:14 (Diff revision 2) > +template <class Iter, class Pred> > +constexpr bool AllOf(Iter aFirst, Iter aLast, Pred p) This method needs some sort of documentation comment.
Attachment #8962745 -
Flags: review?(nfroyd)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237180 > Please add a one-line comment summarizing the purpose of this file (I guess a polyfill for `<algorithm>`)? Added a comment. > Do uses of this in the baseline compiler GCC builds cause errors? Make this function non-constexpr and added ConstExprAllOf (until bug 1449066 is fixed). > This method needs some sort of documentation comment. Added a comment.
![]() |
||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237692 Please add some tests for these new functions. ::: mfbt/Algorithm.h:12 (Diff revision 3) > +/* A polyfill for `<algorithm>`. */ > + > +#ifndef mozilla_Algorithm_h > +#define mozilla_Algorithm_h > + > +#ifdef __cplusplus Do we need the `__cplusplus` include guards? I would think we wouldn't have any C code that would require this header...
Attachment #8962745 -
Flags: review?(nfroyd)
![]() |
||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237692 Apologies for forgetting to ask about this the first time around. :(
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review237692 Added a CppUnitTest. > Do we need the `__cplusplus` include guards? I would think we wouldn't have any C code that would require this header... Sure. Removed the extra #ifdefs.
![]() |
||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8962745 [details] Bug 1449094 - Implement constexpr mozilla::AllOf. https://reviewboard.mozilla.org/r/231616/#review238000 Thank you!
Attachment #8962745 -
Flags: review?(nfroyd) → review+
Comment 13•7 years ago
|
||
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/577c20417552 Implement constexpr mozilla::AllOf. r=froydnj
Comment 14•7 years ago
|
||
Backed out changeset 577c20417552 (bug 1449094) for failing /builds/worker/workspace/build/src/mfbt/tests/TestAlgorithm.cpp Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171128627&repo=autoland Backout push: https://hg.mozilla.org/integration/autoland/rev/2539ded2a075507d76408feb079ddbf5472195f3 https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=577c204175529bf4f23c7736bd67552b393dcc27&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success&filter-classifiedState=unclassified
Flags: needinfo?(VYV03354)
Assignee | ||
Comment 15•7 years ago
|
||
Mmm, constexpr std::begin/end are also new feature since C++14... I'll use plain pointers instead of iterators.
Flags: needinfo?(VYV03354)
Assignee | ||
Comment 16•7 years ago
|
||
GCC 4.9 is really buggy about constexpr... I could not make the test work with GCC 4.9. I have to wait until GCC 4.9 is dropped :(
Depends on: 1449066
Updated•7 years ago
|
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a37c3eab3bebdee2bdc70ec856bbf0069edca0ca
Comment 19•7 years ago
|
||
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/a5111bd8a4f6 Implement constexpr mozilla::AllOf. r=froydnj
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a5111bd8a4f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•