Masatoshi Kimura [:emk] Assignee
	Description • 7 years ago

Until C++20 constexpr all_of is available everywhere.

	Nathan Froyd [:froydnj]
	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?

	Masatoshi Kimura [:emk] 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`.

	Nathan Froyd [:froydnj]
	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.

	Masatoshi Kimura [:emk] 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.

	Nathan Froyd [:froydnj]
	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...

	Nathan Froyd [:froydnj]
	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. :(

	Masatoshi Kimura [:emk] 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.

	Nathan Froyd [:froydnj]
	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!

	Pulsebot
	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

	Bogdan Tara[:bogdan_tara | bogdant]
	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

	Masatoshi Kimura [:emk] 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.

	Masatoshi Kimura [:emk] 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 :(

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 7 years ago

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

	Pulsebot
	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

	Cosmin Sabou [:CosminS]
	Comment 20 • 7 years ago
bugherder

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