1449094 - Implement constexpr mozilla::AllOf

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Masatoshi Kimura [:emk]]

Until C++20 constexpr all_of is available everywhere.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1449094 - Implement constexpr mozilla::AllOf.

Review commit: https://reviewboard.mozilla.org/r/231616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231616/

Comment 2

[Nathan Froyd [:froydnj]]

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?

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231616/diff/1-2/

Comment 4

[Masatoshi Kimura [:emk]]

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

[Nathan Froyd [:froydnj]]

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.

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231616/diff/2-3/

Comment 7

[Masatoshi Kimura [:emk]]

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

[Nathan Froyd [:froydnj]]

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...

Comment 9

[Nathan Froyd [:froydnj]]

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 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231616/diff/3-4/

Comment 11

[Masatoshi Kimura [:emk]]

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

[Nathan Froyd [:froydnj]]

Comment on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

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

Thank you!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/577c20417552
Implement constexpr mozilla::AllOf. r=froydnj

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 15

[Masatoshi Kimura [:emk]]

Mmm, constexpr std::begin/end are also new feature since C++14... I'll use plain pointers instead of iterators.

Comment 16

[Masatoshi Kimura [:emk]]

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 :(

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 8962745 [details]
Bug 1449094 - Implement constexpr mozilla::AllOf.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231616/diff/4-5/

Comment 18

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a37c3eab3bebdee2bdc70ec856bbf0069edca0ca

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5111bd8a4f6
Implement constexpr mozilla::AllOf. r=froydnj

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a5111bd8a4f6