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)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor][necko-active])

Attachments

(1 file)

Bug 1392643 Turn on c++14 for MinGW globally
59 bytes, text/x-review-board-request
bagder
: review+
froydnj
: review+
Details 
>  /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 {
Whiteboard: [tor] → [tor][necko-active]
Attachment #8899872 - Flags: review?(daniel)
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+
Keywords: checkin-needed
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)
Attachment #8899872 - Flags: review?(gps) → review?(nfroyd)
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 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 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+
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)
(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)
That sounds good to me!
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?
Depends on: C++14
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Tom, is this ready to land?
Flags: needinfo?(tom)
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
Okay, this is ready for re-review.
Flags: needinfo?(nfroyd)
(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)
Keywords: checkin-needed
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: