(In reply to Armin K. from comment #0)
> Problem happens because glibmm is built using -std=c++11, while on libc++,
> std::index_sequence is only available if using -std=c++14. Using
> CXX="clang++ -std=c++14" gets glib[mm] to compile.

libc++ is more correct here; index_sequence was only formally added in C++14.

This was presumably missed because libstdc++ is conditional on C++11 for defining index_sequence. That's clearly wrong IMO, especially if you compile with noext like we do, but oh well.

https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/utility#L298

Created attachment 359768 [details] [review]
configure: Require C++14 for std::index_sequence

    We now use std::index_sequence in variant.h, which debuted in C++14.

    This was presumably missed because libstdc++ defines it in C++11 mode.

Created attachment 359769 [details] [review]
examples/thread: Fix a TODO now that we use C++14

    make_unique<T>(…) is more concise/efficient than unique_ptr<T>(new T{…})

Note that requiring c++14 in glibmm requires forcing c++14 on all other mm components, such as atkmm, pangomm, gtkmm (2 and 3) and all the packages that use any of these (I had to do it for gparted (using gtkmm2) and gnome-system-monitor (using gtkmm3)).

Right. To be clear, I don't know if we want to make that change yet. The alternative appears to be replacing the uses of std::index_sequence with a local implementation.

Your warnings are completely correct, and ideally would have been noticed when Bug 777791 was adding this. However, I can't really blame that too much when libstdc++ behaves in a non-standard way and thus made this *seem* to be allowed in C++11.

Since this apppears to be one, I filed it as a bug with libstdc++: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82212

(In reply to Daniel Boles from comment #2)
> This was presumably missed because libstdc++ is conditional on C++11 for
> defining index_sequence. That's clearly wrong IMO, especially if you compile
> with noext like we do, but oh well.
> 
> https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/
> utility#L298

Try reading the code again, or try compiling an example using -std=c++11.

This is not a bug in libstdc++.

It seems you're right. Sorry for the noise.

This patch originated in master, which already requires C++14, but then got picked to 2.54, which does not - and somehow no one noticed until now.

It's not clear from the OP whether g++ still works here, but since a simple test fails as it should, if g++ was working for 2.54, then something else must be up.

GCC defaults to -std=gnu++14 so unless you add an explicit -std=c++11 or -std=gnu++11 you'll get access to C++14 features.

If you do add an explicit -std=c++11 and are still getting C++14 then you must be adding another -std later in the command line to override it (or doing something *really* bad like redefining __cplusplus!)

We call this in configure.ac for 2.54:

  MM_AX_CXX_COMPILE_STDCXX([11], [noext], [mandatory])

which I would've expected to suffice, but yep, maybe something else is going wrong

Thanks for the info!

Sorry for not noticing this before releasing a tarball.

(In reply to Armin K. from comment #5)
> Note that requiring c++14 in glibmm requires forcing c++14 on all other mm
> components, such as atkmm, pangomm, gtkmm (2 and 3) and all the packages
> that use any of these (I had to do it for gparted (using gtkmm2) and
> gnome-system-monitor (using gtkmm3)).

Let's require C++14, instead of C++11, for glibmm 2.54.x. That doesn't seem like a big change. Any practical objections?

Not sure about practical - I'm all for encouraging C++14 - but theoretically, isn't that a bit excessive for a minor release?

It would probably be better to roll our own implementation of index_sequence and move to that, and keep the current (previous) compiler flags.

Here's a page about doing that in C++11, including examples that are specifically about unpacking tuples, like the new feature that introduced this:

https://blog.galowicz.de/2016/06/24/integer_sequences_at_compile_time/

Created attachment 359806 [details] [review]
configure: Require C++14 for std::index_sequence

We now use std::index_sequence in variant.h, which debuted in C++14.


--

Remove the bit blaming libstdc++, since it seems something else happened.

> isn't that a bit excessive for a minor release?

Well, we already require it for the .0 release, so now we are just making that more obvious for a .1 release.

I'd generally like to not have too many branches on too many different C++ versions, and I'd like us to generally push forwards as long as it's no great disadvantage to anyone.

(In reply to Murray Cumming from comment #16)
> > isn't that a bit excessive for a minor release?
> 
> Well, we already require it for the .0 release, so now we are just making
> that more obvious for a .1 release.

Sure, though what I really meant was that maybe we should consider .0 requiring it as an accident, replace the offending class with a small local implementation, and not impose that all users must now bump their own build systems. Maybe no one will mind... but I'm not so certain.

But yeah, if you view it as too late now, that makes sense in its own way too.

I also think it's OK to require it in the change between glibmm 2.52 and 2.54. But I'm open to a practical objection.

Created attachment 359869 [details] [review]
patch: Glib::Variant<std::tuple>: Don't use std::index_sequence from C++14

This patch copies index_sequence from the utility file in libstdc++ to
variant.hg. I don't know if this is legal.

index_sequence is not used in public API. It's only used in templates.
AFAICT the patch does not break API or ABI.

Review of attachment 359869 [details] [review]:

Seems like a good solution. I don't see why there would be any issue of legality, since you're defining these in our own namespace.

It may be nicer to keep this in its own header file, though? Especially if we end up wanting to use it for anything else in this version (however likely that may be).

::: glib/src/variant.hg
@@ +1513,3 @@
+  // Stores a tuple of indices.  Used by tuple and pair, and by bind() to
+  // extract the elements in a tuple.
+  template<size_t... Indexes> struct Index_tuple { };

I think technically we need to #include <cstddef> and use std::size_t everywhere. Plain size_t is not guaranteed to be defined in the global namespace, especially if we don't include any header defining it. It probably will be in most situations, but we should be explicit/correct.

Just as a small remark, this release made it to Debian/sid already. Since Inkscape requires C++11 it's no longer possible to build that. I would be happy if the 2.54 branch could stick with C++11.

Having thought about it more, and despite having written patches to make it happen... my opinion is this.

Suddenly, unexpectedly - and ultimately unintentionally - requiring all users to start using a different C++ revision - which will happen, even if they don't try to use the new Variant features - may be seen as highly inconvenient and not a source of goodwill.

Plus, as if that's not reason enough - which it should be - it's possible, however rare in practice, that they will have to do more than update one number somewhere, since in theory changes in C++14 can break code that was valid in C++11. So, there might be more than the immediate effect on projects using this. See https://www.infoq.com/news/2014/06/cpp14-changes-breaking-compat and the SO thread linked from it.

IMO, given how this dependency on C++14 was inadvertent, and how simple it is to include a little local implementation of index_sequence, there's no reason not to fix the accident and put out a .1 asap, clarifying that C++11 is still OK.


(Then we still have to figure out why apparently everyone except the OP was able to compile this in C++11 mode!)

> I don't see why there would be any issue of legality, since you're defining
> these in our own namespace.

What I meant: According to the copyright notices in the files, libstdc++ is
released under the GNU General Public License, version 3. glibmm is released
under the GNU Lesser General Public License, version 2.1.
Is it allowed to copy some code from libstdc++ to glibmm? If it's at all
allowed, do we have to include some extra copyright notice? E.g. saying that
this code is copyright Free Software Foundation, Inc?

> It may be nicer to keep this in its own header file, though? Especially if we
> end up wanting to use it for anything else in this version.

Let's move it to its own header file only if and when we really find a second
use for it. It's improbable that it will ever be needed.

> I think technically we need to #include <cstddef> and use std::size_t
> everywhere.

Correct. I should have added std:: when I moved the code from namespace std.

(In reply to Kjell Ahlstedt from comment #23)
> > I don't see why there would be any issue of legality, since you're defining
> > these in our own namespace.
> 
> What I meant: According to the copyright notices in the files, libstdc++ is
> released under the GNU General Public License, version 3. glibmm is released
> under the GNU Lesser General Public License, version 2.1.
> Is it allowed to copy some code from libstdc++ to glibmm? If it's at all
> allowed, do we have to include some extra copyright notice? E.g. saying that
> this code is copyright Free Software Foundation, Inc?


No, you cannot relicense GPLv3+exception code as LGPL v2.1 without permission from the copyright holder.

Since I'm the original author of that code, I could give permission to use it in glibmm under the LGPL terms, but I refuse, because it's a *really* weird implementation to copy! It's unnecessarily complicated in order to share the implementation with the _Index_tuple type that we already had in libstdc++ before I standardized std::integer_sequence. You have no need for that.

Let me give you a much simpler form ...

Here's one I made earlier:
https://gitlab.com/redistd/integer_seq/blob/master/integer_seq.h

I started simplifying the libstdc++ version and realised I was reimplementing exactly this. You have my permission to use that code under the terms of the LGPL v2.1 or later.

Or here's the simplified form of the libstdc++ code, without the extra libstdc++-specific stuff you don't need:

#include <cstddef>

  /// Class template integer_sequence
  template<typename T, T... Idx>
    struct integer_sequence
    {
      typedef T value_type;
      static constexpr size_t size() { return sizeof...(Idx); }
    };

  // Concatenates two integer_sequences.
  template<typename Iseq1, typename Iseq2> struct iseq_cat;

  template<typename T, size_t... Ind1, size_t... Ind2>
    struct iseq_cat<integer_sequence<T, Ind1...>, integer_sequence<T, Ind2...>>
    {
      using type = integer_sequence<T, Ind1..., (Ind2 + sizeof...(Ind1))...>;
    };

  // Builds an integer_sequence<T, 0, 1, 2, ..., Num-1>.
  template<typename T, size_t Num>
    struct make_intseq
    : iseq_cat<typename make_intseq<T, Num / 2>::type,
		typename make_intseq<T, Num - Num / 2>::type>
    { };

  template<typename T>
    struct make_intseq<T, 1>
    {
      typedef integer_sequence<T, 0> type;
    };

  template<typename T>
    struct make_intseq<T, 0>
    {
      typedef integer_sequence<T> type;
    };

  /// Alias template make_integer_sequence
  template<typename T, T Num>
    using make_integer_sequence = typename make_intseq<T, Num>::type;

  /// Alias template index_sequence
  template<size_t... Idx>
    using index_sequence = integer_sequence<size_t, Idx...>;

  /// Alias template make_index_sequence
  template<size_t Num>
    using make_index_sequence = make_integer_sequence<size_t, Num>;

  /// Alias template index_sequence_for
  template<typename... Types>
    using index_sequence_for = make_index_sequence<sizeof...(Types)>;

This has been breaking Inkscape's automated nightly PPA builds this week:

Build Log: https://launchpad.net/~inkscape.dev/+archive/ubuntu/trunk/+build/13381797/+files/buildlog_ubuntu-artful-i386.inkscape-trunk_1%3A0.92.0+devel+16029+91~ubuntu17.10.1_BUILDING.txt.gz

If the fix won't be available soonish (within a few days), is there a recommended workaround?

Created attachment 359924 [details] [review]
patch: Glib::Variant<std::tuple>: Don't use std::index_sequence from C++14

Hopefully a legal patch this time. This patch uses Jonathan's code from
comment 27. Thanks a lot for that code!

All this trouble with index_sequence is my fault, I'm afraid. I copied a patch
from the master branch without realizing that the patch required C++14 support.
That's no problem in the master branch, but it is in the glibmm-2-54 branch.

MM_AX_CXX_COMPILE_STDCXX([11], [noext], [mandatory]) in configure.ac prefers a
configuration with the default value of the -std parameter in g++. The default
of g++ version 6 is to support C++14. Therefore the compiler did not report the
problematic use of std::index_sequence when I built glibmm.
The MM_AX_CXX_COMPILE_STDCXX() macro seems to generate configuration code that
gives you support for *at least* what you require, not *exactly* what you
require.

Jonathan, can you please confirm that it's OK with you that I push the patch in
comment 29 to glibmm. Or, if it's not OK, tell which modifications you want.

Yes that's fine, just don't use the overly complicated one straight from libstdc++, as that solves a different problem.

This is the commit that used std::index_sequence:
https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-54&id=cca6a67aeef42a79c9b7896ab092e513a779a8b3

Wouldn't it be simpler to just revert it?

> Wouldn't it be simpler to just revert it?

Then the Glib::Variant<std::tuple> specialization that was added in
glibmm 2.54.0 would be removed in 2.54.1. If that's acceptable, reverting the
commit is of course an alternative.

Thanks to Jonathan, we now have a patch that fixes the bug without removing
Glib::Variant<std::tuple>. A few days ago it would have been simpler to revert
the problematic commit. Now it's just as simple to commit the new patch.

I have pushed the patch.
https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-54&id=7f825b98c6ba261ffe1117ec80e3b150de424bcd

I suppose that we must also very soon release glibmm 2.54.1 with this fix.
Murray, will you do that, or do you want me to do it?

OK. Thanks. Please go ahead.

glibmm 2.54.1 with the fix of this bug has been released.

While I'm able to build glibmm and friends without using -std=c++14 now, there's still one minor warning:

In file included from pollableoutputstream.cc:4:
In file included from ../../glib/glibmm.h:92:
In file included from ../../glib/glibmm/arrayhandle.h:21:
In file included from ../../glib/glibmm/containerhandle_shared.h:23:
../../glib/glibmm/variant.h:2132:3: warning: use of this statement in a constexpr function is a C++14 extension [-Wc++14-extensions]
  (void)arg;
  ^

clang++ issues more warnings than g++. I should have tested with clang++
earlier. Now I've pushed a patch that fixes the (void)arg warning and another
warning from clang++.

variant.cc:267:40: error: implicit conversion of nullptr constant to 'bool'
      [-Werror,-Wnull-conversion]
  return gobj() ? GINT_TO_POINTER(1) : nullptr;

https://git.gnome.org/browse/glibmm/commit/?h=glibmm-2-54&id=7f5065a7de446563f303fbbdb5d28a710403c23f

How urgent is this fix? Do you need a glibmm 2.54.2 release right away?
If it's not that urgent, I'd prefer to wait for a while. There might be other
fixes that could go into 2.54.2. ("A while" in this case could means several
weeks.)

It's not a big deal, it's just a warning. Take your time for .2 release.