Closed
Bug 1392643
Opened 7 years ago
Closed 7 years ago
error: lambda capture initializers only available with -std=c++14 or -std=gnu++14
Categories
(Core :: Networking, defect, P5)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tor][necko-active])
Attachments
(1 file)
> /home/worker/workspace/build/src/netwerk/socket/nsNamedPipeService.cpp:46:53: error: lambda capture initializers only available with -std=c++14 or -std=gnu++14 [-Werror]
> [self = Move(self)] () -> void {
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
Non-MinGW Try run just to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a96c3b496aeee157ecc4899712c5e5f2fdfb318
![]() |
||
Updated•7 years ago
|
Whiteboard: [tor] → [tor][necko-active]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8899872 -
Flags: review?(daniel)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review177912
Attachment #8899872 -
Flags: review?(daniel) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 5•7 years ago
|
||
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Attachment #8899872 -
Flags: review?(hurley)
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review178796 This all looks fine from a necko standpoint (and indeed, :bagder being a necko peer has already r+'d it). However, given I'm not familiar with the build system stuff for this (specifically what we can rely on being available in certain circumstances) I'd like to have a build peer take a look.
Attachment #8899872 -
Flags: review?(hurley)
Attachment #8899872 -
Flags: review?(gps)
Updated•7 years ago
|
Attachment #8899872 -
Flags: review?(gps) → review?(nfroyd)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review179650 I'm not sure if this is safe given our minimal toolchain requirements. So punting to someone who should know.
![]() |
||
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review179656 It is obviously safe to do this regardless of our minimum toolchain requirements, since this code only gets activated for mingw and AFAIK we don't have toolchain requirements for mingw. OK, so the reason we don't see this with our normal builds is that: 1) This code is Windows-only, so it's only ever compiled with MSVC and clang-cl; 2) MSVC 2015 silently enables C++14 features, and we do the same for clang-cl: http://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#380 which explains why nothing complains, as the use of C++14 features is fine here. I'm not super excited about starting to add `-std=FOO` flags all over the tree, even if they're targetted at (currently) tier3 platforms; my concern is that people are going to see this and start adding `-std=FOO` flags in their directory just so they can use features from newer standards. I don't think that would be a great state of affairs. I can think of two ways to fix this that are more appealing: 1) Fix the nsNamedPipeService code to just capture the parameter normally by value, deleting the move. I think this should be OK, as we have similar patterns in other places we use refcounted objects with lambdas. 2) Fix the configure code around the location above to pass `-std=c++14` for mingw, which may also wind up heading off a lot of other issues similar to this one.
Attachment #8899872 -
Flags: review?(nfroyd) → review-
Comment hidden (mozreview-request) |
![]() |
||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review181882 This is better, thank you! A word of warning: there are moderately extensive tests for the toolchain selection stuff; you'll want to run something like `mach python-test python/mozbuild/mozbuild/test/configure` (or do an all-platforms build-only try run) before you land.
Attachment #8899872 -
Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Try run with tests fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77cb6b63da6159a939617722397dc1326c65ad0e
Assignee | ||
Comment 13•7 years ago
|
||
Try run looked good, but would like you to look at the test change. The one thing I am unsure of is I said "g++ has std++14 support" which is... mostly true for 4.9; but not true for 4.7. https://gcc.gnu.org/projects/cxx-status.html#cxx14 I'm not sure if the nuance is important enough to correct...
Flags: needinfo?(nfroyd)
![]() |
||
Comment 14•7 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #13) > Try run looked good, but would like you to look at the test change. The one > thing I am unsure of is I said "g++ has std++14 support" which is... mostly > true for 4.9; but not true for 4.7. > https://gcc.gnu.org/projects/cxx-status.html#cxx14 I'm not sure if the > nuance is important enough to correct... I remember now that I had to touch these tests for bug 1325632; bug 1325632 comment 2 and bug 1325632 comment 5 may be of interest to you. In short, we want to make the fake compilers in those tests look as much like the actual compilers as possible, so GCC 4.7 should not advertise std++14 support, while GCC 4.9 and above should. Doing this requires changes in other places. I could land part 1 from that bug for you, if you like, and then you can crib the various compiler setup bits in the tests from part 2 (but not the actual result changes, I think) for your patch. Does that make sense?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 15•7 years ago
|
||
That sounds good to me!
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8899872 [details] Bug 1392643 Turn on c++14 for MinGW globally https://reviewboard.mozilla.org/r/171204/#review182532 ::: build/moz.configure/toolchain.configure:382 (Diff revision 4) > if info.language == 'C++': > + if target.kernel != 'WINNT': > - if info.type in ('clang', 'gcc') and info.language_version != 201103: > + if info.type in ('clang', 'gcc') and info.language_version != 201103: > - append_flag('-std=gnu++11') > + append_flag('-std=gnu++11') > + else: > + if info.type in ('clang') and info.language_version != 201103: `in ('clang')` is the same as `in 'clang'`. while in practice, that will work, it's not the test you want. just use ==. OTOH, shouldn't an hypothetical clang build for windows have the same c++14 problem as gcc?
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Assignee | ||
Comment 20•7 years ago
|
||
No. (It isn't P1 either) I'm waiting on a piece of Bug 1325632 to land then I can refactor and land this one. I'll talk to Nathan about the dependency soon.
Flags: needinfo?(tom)
Priority: P1 → P5
Comment hidden (mozreview-request) |
![]() |
||
Comment 23•7 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #22) > Okay, this is ready for re-review. New revision looks great, thank you!
Flags: needinfo?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 24•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b81d24ced513 Turn on c++14 for MinGW globally r=bagder,froydnj
Keywords: checkin-needed
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b81d24ced513
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•