Closed Bug 1449094 Opened 2 years ago Closed 2 years ago

Implement constexpr mozilla::AllOf

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

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.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
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 on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

https://reviewboard.mozilla.org/r/231616/#review237150

Oops, I forgot `hg add`.
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 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 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 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 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 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+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/577c20417552
Implement constexpr mozilla::AllOf. r=froydnj
Mmm, constexpr std::begin/end are also new feature since C++14... I'll use plain pointers instead of iterators.
Flags: needinfo?(VYV03354)
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
Depends on: gcc-6.1
No longer depends on: 1449066
Depends on: 1444271
No longer depends on: gcc-6.1
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/a5111bd8a4f6
Implement constexpr mozilla::AllOf. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/a5111bd8a4f6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.