error with d28016d16e9a (change to jsonencode.cc)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

error with d28016d16e9a (change to jsonencode.cc)

Octave - Maintainers mailing list
Rik

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

If I revert the change below, I’m able to build.

http://hg.savannah.gnu.org/hgweb/octave/rev/d28016d16e9a

libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -Iliboctave -I./liboctave -I./liboctave/array -Iliboctave/numeric -I./liboctave/numeric -Iliboctave/operators -I./liboctave/operators -I./liboctave/system -I./liboctave/util -I./libinterp/octave-value -Ilibinterp -I./libinterp -I./libinterp/operators -Ilibinterp/parse-tree -I./libinterp/parse-tree -Ilibinterp/corefcn -I./libinterp/corefcn -I./liboctave/wrappers -I/usr/local/opt/hdf5/include -I/usr/local/Cellar/graphicsmagick/1.3.35/include/GraphicsMagick -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fontconfig/2.13.1/include -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/hdf5/include -I/usr/local/opt/readline/include -I/usr/local/opt/sqlite/include -I/usr/local/opt/openssl/include -I/usr/local/opt/gettext/include -I/usr/local/opt/icu4c/include -I/usr/local/opt/qt5/include -I/usr/local/opt/sundials27/include -I/usr/local/opt/zlib/include -fPIC -D_THREAD_SAFE -pthread -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -g -O2 -MT libinterp/corefcn/libcorefcn_la-jsonencode.lo -MD -MP -MF libinterp/corefcn/.deps/libcorefcn_la-jsonencode.Tpo -c libinterp/corefcn/jsonencode.cc  -fno-common -DPIC -o libinterp/corefcn/.libs/libcorefcn_la-jsonencode.o
libinterp/corefcn/jsonencode.cc:414:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:414:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
libinterp/corefcn/jsonencode.cc:425:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:425:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
In file included from libinterp/corefcn/jsonencode.cc:32:
In file included from libinterp/corefcn/error.h:35:
./liboctave/util/unwind-prot.h:171:9: error: no matching constructor for initialization of 'std::function<void ()>'
      : m_fcn (std::bind (fcn, args...))
        ^      ~~~~~~~~~~~~~~~~~~~~~~~~

Ben
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Rik-4
On 10/01/2020 08:26 AM, Benjamin Abbott wrote:
Rik

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

If I revert the change below, I’m able to build.

http://hg.savannah.gnu.org/hgweb/octave/rev/d28016d16e9a

libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -Iliboctave -I./liboctave -I./liboctave/array -Iliboctave/numeric -I./liboctave/numeric -Iliboctave/operators -I./liboctave/operators -I./liboctave/system -I./liboctave/util -I./libinterp/octave-value -Ilibinterp -I./libinterp -I./libinterp/operators -Ilibinterp/parse-tree -I./libinterp/parse-tree -Ilibinterp/corefcn -I./libinterp/corefcn -I./liboctave/wrappers -I/usr/local/opt/hdf5/include -I/usr/local/Cellar/graphicsmagick/1.3.35/include/GraphicsMagick -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fontconfig/2.13.1/include -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/hdf5/include -I/usr/local/opt/readline/include -I/usr/local/opt/sqlite/include -I/usr/local/opt/openssl/include -I/usr/local/opt/gettext/include -I/usr/local/opt/icu4c/include -I/usr/local/opt/qt5/include -I/usr/local/opt/sundials27/include -I/usr/local/opt/zlib/include -fPIC -D_THREAD_SAFE -pthread -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -g -O2 -MT libinterp/corefcn/libcorefcn_la-jsonencode.lo -MD -MP -MF libinterp/corefcn/.deps/libcorefcn_la-jsonencode.Tpo -c libinterp/corefcn/jsonencode.cc  -fno-common -DPIC -o libinterp/corefcn/.libs/libcorefcn_la-jsonencode.o
libinterp/corefcn/jsonencode.cc:414:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:414:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
libinterp/corefcn/jsonencode.cc:425:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:425:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
In file included from libinterp/corefcn/jsonencode.cc:32:
In file included from libinterp/corefcn/error.h:35:
./liboctave/util/unwind-prot.h:171:9: error: no matching constructor for initialization of 'std::function<void ()>'
      : m_fcn (std::bind (fcn, args...))
        ^      ~~~~~~~~~~~~~~~~~~~~~~~~

Ben

In one sense, this is easy to resolve.  Instead of using the keyword "auto" just specify the exact type of the underlying variable.  However, the compiler really should be able to handle this construct.  Could you try 'g++ --version'?  And is it really GNU g++ or is this a soft link to clang?

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Rik-4
In reply to this post by Octave - Maintainers mailing list


On 10/01/2020 10:24 AM, Rik wrote:
On 10/01/2020 08:26 AM, Benjamin Abbott wrote:
Rik

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

If I revert the change below, I’m able to build.

http://hg.savannah.gnu.org/hgweb/octave/rev/d28016d16e9a

libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -Iliboctave -I./liboctave -I./liboctave/array -Iliboctave/numeric -I./liboctave/numeric -Iliboctave/operators -I./liboctave/operators -I./liboctave/system -I./liboctave/util -I./libinterp/octave-value -Ilibinterp -I./libinterp -I./libinterp/operators -Ilibinterp/parse-tree -I./libinterp/parse-tree -Ilibinterp/corefcn -I./libinterp/corefcn -I./liboctave/wrappers -I/usr/local/opt/hdf5/include -I/usr/local/Cellar/graphicsmagick/1.3.35/include/GraphicsMagick -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fontconfig/2.13.1/include -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/hdf5/include -I/usr/local/opt/readline/include -I/usr/local/opt/sqlite/include -I/usr/local/opt/openssl/include -I/usr/local/opt/gettext/include -I/usr/local/opt/icu4c/include -I/usr/local/opt/qt5/include -I/usr/local/opt/sundials27/include -I/usr/local/opt/zlib/include -fPIC -D_THREAD_SAFE -pthread -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -g -O2 -MT libinterp/corefcn/libcorefcn_la-jsonencode.lo -MD -MP -MF libinterp/corefcn/.deps/libcorefcn_la-jsonencode.Tpo -c libinterp/corefcn/jsonencode.cc  -fno-common -DPIC -o libinterp/corefcn/.libs/libcorefcn_la-jsonencode.o
libinterp/corefcn/jsonencode.cc:414:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:414:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
libinterp/corefcn/jsonencode.cc:425:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:425:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
In file included from libinterp/corefcn/jsonencode.cc:32:
In file included from libinterp/corefcn/error.h:35:
./liboctave/util/unwind-prot.h:171:9: error: no matching constructor for initialization of 'std::function<void ()>'
      : m_fcn (std::bind (fcn, args...))
        ^      ~~~~~~~~~~~~~~~~~~~~~~~~

Ben

In one sense, this is easy to resolve.  Instead of using the keyword "auto" just specify the exact type of the underlying variable.  However, the compiler really should be able to handle this construct.  Could you try 'g++ --version'?  And is it really GNU g++ or is this a soft link to clang?


Oops, completely missed that you had already provided the version at the top.  I happen to be using g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 where this works correctly.  I think the issue is that 'auto' in lambdas became acceptable only in C++14.  Could you try adding the flag '-std=gnu++14' and see if it compiles?  If it does that will explain the difference, but we then have a decision about how old a compiler we want to support.  2014 is six years ago, and it will be seven years by the time the development version of Octave becomes 7.1 so it might be acceptable to move up.

--Rik


Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Carlo de Falco-2
In reply to this post by Rik-4


> Il giorno 1 ott 2020, alle ore 19:24, Rik <[hidden email]> ha scritto:
>
> In one sense, this is easy to resolve.  Instead of using the keyword "auto" just specify the exact type of the underlying variable.  However, the compiler really should be able to handle this construct.  Could you try 'g++ --version'?  And is it really GNU g++ or is this a soft link to clang?

Isn't this a C++14 feature?

the following simple example compiles for me with std=c++14 or std=c++17 but fails with std=c++11 or std=gnu++11 :

int main ()
{
  auto f = [] (auto x) { return x + 1; };
  f (1.0);
}


$ clang++ -std=gnu++11 pippo.cc -o pippo
pippo.cc:3:16: error: 'auto' not allowed in lambda parameter
  auto f = [] (auto x) { return x + 1; };
               ^~~~
1 error generated.

> --Rik

c.
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Carlo de Falco-2
In reply to this post by Octave - Maintainers mailing list


Il giorno 1 ott 2020, alle ore 17:26, Benjamin Abbott via Octave-maintainers <[hidden email]> ha scritto:

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

does it work if you try building with CXXFLAGS="-std=c++14 "?

c.
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

John W. Eaton
Administrator
In reply to this post by Rik-4
On 10/1/20 1:24 PM, Rik wrote:

> In one sense, this is easy to resolve.  Instead of using the keyword
> "auto" just specify the exact type of the underlying variable. However,
> the compiler really should be able to handle this construct.  Could you
> try 'g++ --version'?  And is it really GNU g++ or is this a soft link to
> clang?

It looks like accepting auto in this context is a C++14 feature.

Also, I noticed we can simplify some things.  The unwind_action allows
writing things like

   int val;  // Will be captured by the unwind_action objects below.

   unwind_action act ([val] () { use_val (val); })

or

   unwind_action act ([] (int val) { use_val (val); }, val);

in the first case, VAL is captured directly by the lambda expression and
is not passed as an argument to the function it creates.  In the second,
the unwind_action object captures the value and passes it as an argument
to the function that the lambda expression creates.

Both forms have the same ultimate effect, but the second is useful when
you want to capture a value that is not directly available as a variable
and can't be captured by the lambda.  For example, in

   octave::unwind_action restore_warning_state
     ([] (const octave_value_list& old_warning_state)
      {
        set_warning_state (old_warning_state);
      }, set_warning_state ("Octave:classdef-to-struct", "off"));

the warning state is only accessible through function calls in this
context, so we can't capture it directly in the [] part of the lambda
expression, but in other places, we could write

   octave::unwind_action close_outfile
     ([outfile] () { std::fclose (outfile); });

instead of

   octave::unwind_action close_outfile
     ([] (const auto file_ptr) { std::fclose (file_ptr); }, outfile);

Using either form should work but maybe we should use the direct lambda
capture form when the values we need to capture are variables accessible
in the current scope?

jwe

Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Rik-4
On 10/01/2020 10:57 AM, John W. Eaton wrote:
On 10/1/20 1:24 PM, Rik wrote:

In one sense, this is easy to resolve.  Instead of using the keyword "auto" just specify the exact type of the underlying variable. However, the compiler really should be able to handle this construct.  Could you try 'g++ --version'?  And is it really GNU g++ or is this a soft link to clang?

It looks like accepting auto in this context is a C++14 feature.

Also, I noticed we can simplify some things.  The unwind_action allows writing things like

  int val;  // Will be captured by the unwind_action objects below.

  unwind_action act ([val] () { use_val (val); })

or

  unwind_action act ([] (int val) { use_val (val); }, val);

in the first case, VAL is captured directly by the lambda expression and is not passed as an argument to the function it creates.  In the second, the unwind_action object captures the value and passes it as an argument to the function that the lambda expression creates.

Both forms have the same ultimate effect, but the second is useful when you want to capture a value that is not directly available as a variable and can't be captured by the lambda.  For example, in

  octave::unwind_action restore_warning_state
    ([] (const octave_value_list& old_warning_state)
     {
       set_warning_state (old_warning_state);
     }, set_warning_state ("Octave:classdef-to-struct", "off"));

the warning state is only accessible through function calls in this context, so we can't capture it directly in the [] part of the lambda expression, but in other places, we could write

  octave::unwind_action close_outfile
    ([outfile] () { std::fclose (outfile); });

instead of

  octave::unwind_action close_outfile
    ([] (const auto file_ptr) { std::fclose (file_ptr); }, outfile);

Using either form should work but maybe we should use the direct lambda capture form when the values we need to capture are variables accessible in the current scope?

"should work", but needs to be checked.  The first lambda expression I wrote for Octave would not work with capture which is why I used the parameter-passing approach.  I then coded the rest of the lambda expressions using the same template; perhaps that was lazy of me.  In any case, it's worth paying attention to the variable that is getting passed and not bother with passing a pointer by reference as that will merely create a double indirection.  Maybe the compiler would get rid of that, but it's better not to rely on that.

--Rik
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Octave - Maintainers mailing list
In reply to this post by Rik-4
On Oct 1, 2020, at 10:24 AM, Rik <[hidden email]> wrote:

On 10/01/2020 08:26 AM, Benjamin Abbott wrote:
Rik

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

If I revert the change below, I’m able to build.

http://hg.savannah.gnu.org/hgweb/octave/rev/d28016d16e9a

libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -Iliboctave -I./liboctave -I./liboctave/array -Iliboctave/numeric -I./liboctave/numeric -Iliboctave/operators -I./liboctave/operators -I./liboctave/system -I./liboctave/util -I./libinterp/octave-value -Ilibinterp -I./libinterp -I./libinterp/operators -Ilibinterp/parse-tree -I./libinterp/parse-tree -Ilibinterp/corefcn -I./libinterp/corefcn -I./liboctave/wrappers -I/usr/local/opt/hdf5/include -I/usr/local/Cellar/graphicsmagick/1.3.35/include/GraphicsMagick -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fftw/3.3.8_2/include -I/usr/local/Cellar/fontconfig/2.13.1/include -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/freetype/include/freetype2 -I/usr/local/opt/hdf5/include -I/usr/local/opt/readline/include -I/usr/local/opt/sqlite/include -I/usr/local/opt/openssl/include -I/usr/local/opt/gettext/include -I/usr/local/opt/icu4c/include -I/usr/local/opt/qt5/include -I/usr/local/opt/sundials27/include -I/usr/local/opt/zlib/include -fPIC -D_THREAD_SAFE -pthread -Wall -W -Wshadow -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -g -O2 -MT libinterp/corefcn/libcorefcn_la-jsonencode.lo -MD -MP -MF libinterp/corefcn/.deps/libcorefcn_la-jsonencode.Tpo -c libinterp/corefcn/jsonencode.cc  -fno-common -DPIC -o libinterp/corefcn/.libs/libcorefcn_la-jsonencode.o
libinterp/corefcn/jsonencode.cc:414:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:414:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
libinterp/corefcn/jsonencode.cc:425:20: error: 'auto' not allowed in lambda parameter
        ([] (const auto& old_warning_state)
                   ^~~~
libinterp/corefcn/jsonencode.cc:425:26: warning: unused parameter 'old_warning_state' [-Wunused-parameter]
        ([] (const auto& old_warning_state)
                         ^
In file included from libinterp/corefcn/jsonencode.cc:32:
In file included from libinterp/corefcn/error.h:35:
./liboctave/util/unwind-prot.h:171:9: error: no matching constructor for initialization of 'std::function<void ()>'
      : m_fcn (std::bind (fcn, args...))
        ^      ~~~~~~~~~~~~~~~~~~~~~~~~

Ben

In one sense, this is easy to resolve.  Instead of using the keyword "auto" just specify the exact type of the underlying variable.  However, the compiler really should be able to handle this construct.  Could you try 'g++ --version'?  And is it really GNU g++ or is this a soft link to clang?

—Rik


Hi Rik,

It’s clang

$ g++ --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple clang version 12.0.0 (clang-1200.0.32.2)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Ben

Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

John W. Eaton
Administrator
In reply to this post by Rik-4
On 10/1/20 2:33 PM, Rik wrote:

> "should work", but needs to be checked.  The first lambda expression I
> wrote for Octave would not work with capture which is why I used the
> parameter-passing approach.

Do you remember what it was that didn't work?

Except for "this", lambdas capture by value by default.  Capturing
"this" allows access to the current object, so instead of writing

   unwind_action restore_scope
     ([] (auto self) { self->pop (); }, this);

we can write

   unwind_action restore_scope ([this] (void) { pop (); });

and instead of

   unwind_action executing_callbacks_cleanup
     ([] (auto old_callback_props, auto &self)
      { old_callback_props->erase (self); }, &executing_callbacks, this);

we can write

   unwind_action executing_callbacks_cleanup
     ([this] () { executing_callbacks.erase (this); });

In this last case, "executing_callbacks" is also a file-scope static
variable, so the lambda can access it directly without capture.

We could also decide to always use "[=]" when we need to capture
something.  Then all variables that are needed, including "this" will be
captured using the default rules for capture by value.

The unwind_action + lambda expression feature is definitely complicated
and has a lot of options for how to use it.  There's no one right way.
I'd just like to agree on a set of guidelines to make it as simple as
possible to write, understand, and maintain.  My first pass at that would be

   * when possible, capture variables by value directly in the lambda
expression instead of the unwind_action object

   * use lambda expression arguments and capture values in the
unwind_action object only when necessary (for example, when the value is
not a variable in the local scope)

and possibly

   * use "[=]" to capture local variables and avoid the redundancy of
explicitly writing the list of captured variables since they will
already appear in the lambda expression anyway

and, once we can move to C++14, maybe also add

   * use "auto" for lambda expression argument declarations

jwe

Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Octave - Maintainers mailing list
In reply to this post by Carlo de Falco-2
On Oct 1, 2020, at 10:43 AM, Carlo De Falco <[hidden email]> wrote:


Il giorno 1 ott 2020, alle ore 17:26, Benjamin Abbott via Octave-maintainers <[hidden email]> ha scritto:

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

does it work if you try building with CXXFLAGS="-std=c++14 "?

c.

Hi Carlo,

Your suggestion worked for me.

Ben
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Rik-4
In reply to this post by John W. Eaton
On 10/01/2020 12:42 PM, John W. Eaton wrote:
On 10/1/20 2:33 PM, Rik wrote:

"should work", but needs to be checked.  The first lambda expression I wrote for Octave would not work with capture which is why I used the parameter-passing approach.

Do you remember what it was that didn't work?

I don't.  It might work now that we've switched to unwind_action rather than unwind_protect objects.


Except for "this", lambdas capture by value by default.  Capturing "this" allows access to the current object, so instead of writing

  unwind_action restore_scope
    ([] (auto self) { self->pop (); }, this);

we can write

  unwind_action restore_scope ([this] (void) { pop (); });

and instead of

  unwind_action executing_callbacks_cleanup
    ([] (auto old_callback_props, auto &self)
     { old_callback_props->erase (self); }, &executing_callbacks, this);

we can write

  unwind_action executing_callbacks_cleanup
    ([this] () { executing_callbacks.erase (this); });

In this last case, "executing_callbacks" is also a file-scope static variable, so the lambda can access it directly without capture.

I like these as even simpler, less verbose forms.


We could also decide to always use "[=]" when we need to capture something.  Then all variables that are needed, including "this" will be captured using the default rules for capture by value.

I don't think this is recommended practice.  The temporary anonymous struct that is created to represent the lambda expression will then have an argument in the constructor for every existing variable in the surrounding function.  The compiler *might* then optimize out all of the additional unused captures, but I think it would be better to just capture what you need either by value "[=variable_name]" if small like a built-in type or a pointer or by reference "[&variable_name]" if it is something large like the instance of a class.  For reference, I was using this https://dzone.com/articles/all-about-lambda-functions-in-cfrom-c11-to-c17.


The unwind_action + lambda expression feature is definitely complicated and has a lot of options for how to use it.  There's no one right way. I'd just like to agree on a set of guidelines to make it as simple as possible to write, understand, and maintain.  My first pass at that would be

  * when possible, capture variables by value directly in the lambda expression instead of the unwind_action object

I would modify this to only small values should be passed by value.


  * use lambda expression arguments and capture values in the unwind_action object only when necessary (for example, when the value is not a variable in the local scope)

This seems like a good convention to adopt.


and possibly

  * use "[=]" to capture local variables and avoid the redundancy of explicitly writing the list of captured variables since they will already appear in the lambda expression anyway

I would skip this as noted above.

--Rik


and, once we can move to C++14, maybe also add

  * use "auto" for lambda expression argument declarations

jwe




Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Carlo de Falco-2
In reply to this post by Octave - Maintainers mailing list


Il giorno 1 ott 2020, alle ore 23:22, Benjamin Abbott via Octave-maintainers <[hidden email]> ha scritto:

On Oct 1, 2020, at 10:43 AM, Carlo De Falco <[hidden email]> wrote:


Il giorno 1 ott 2020, alle ore 17:26, Benjamin Abbott via Octave-maintainers <[hidden email]> ha scritto:

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

does it work if you try building with CXXFLAGS="-std=c++14 "?

c.

Hi Carlo,

Your suggestion worked for me.

Ben


So, if it is now accepted to require a C++14 compatible compiler we should just document that and change the current configure test which looks for C++11 to look for C++14 instead.
Otherwise, the use of "auto" for parameters should be abandoned. I'm not following in detail the discussion on this between jwe and Rik but it seems they are planning some way to work around this ...
c.
Reply | Threaded
Open this post in threaded view
|

lambda expressions (was: Re: error with d28016d16e9a ...)

John W. Eaton
Administrator
In reply to this post by Rik-4
On 10/1/20 8:13 PM, Rik wrote:
> On 10/01/2020 12:42 PM, John W. Eaton wrote:

>> We could also decide to always use "[=]" when we need to capture
>> something.  Then all variables that are needed, including "this" will
>> be captured using the default rules for capture by value.
>
> I don't think this is recommended practice.  The temporary anonymous
> struct that is created to represent the lambda expression will then have
> an argument in the constructor for every existing variable in the
> surrounding function.  The compiler *might* then optimize out all of the
> additional unused captures, but I think it would be better to just
> capture what you need either by value "[=variable_name]" if small like a
> built-in type or a pointer or by reference "[&variable_name]" if it is
> something large like the instance of a class.  For reference, I was
> using this
> https://dzone.com/articles/all-about-lambda-functions-in-cfrom-c11-to-c17.
As I understand it, the lambda expression only captures variables that
are used.  Using a capture default specification actually seems better
to me because the variables are already listed in the lambda expression,
so explicitly listing them again is redundant.  To me, this seems quite
similar to using "auto" to avoid writing out something that the compiler
can figure out.  I don't know why the article you linked lists [=] and
[&] as not recommended.  I didn't see an explanation and I'm not finding
other similar recommendations.

>>   * when possible, capture variables by value directly in the lambda
>> expression instead of the unwind_action object
>
> I would modify this to only small values should be passed by value.

For unwind-protect to work to save and restore values, they must be
copied.  You can't just store a reference to a value that you want to
restore later.  See the attached example.

In Octave, most values are reference counted and relatively cheap to
copy anyway.

OTOH, if you mean to use references to objects that are expensive to
copy and that are needed in by the lambda expression but that aren't
there to be saved/restored, then I agree, it's OK to capture by
reference.  In that case, it would be nice if you could capture by const
reference so you could explicitly say that you won't be modifying the
value inside the lambda expression, but I don't see a way to do that
without using C++14 features (see
https://en.cppreference.com/w/cpp/language/lambda and search for "This
also makes it possible to capture by const reference").

Note that

   unwind_action ([] (const auto& x) { use (x); }, x);

is not capturing X by const reference.  The unwind_action object only
captures by value (copy).  Then, when the unwind_action destructor
executes, it passes the captured value to the function generated by the
lambda expression by const reference.  But the unwind_action object
still grabbed a copy of X.

The following does capture X by reference

   unwind_action ([&x] () { use (x); });

but it is not const, so it is possible to change to X in the lambda
expression function and affect the value of X in the parent scope as well.

jwe

lambda-tst.cc (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

John W. Eaton
Administrator
In reply to this post by Carlo de Falco-2
On 10/2/20 4:49 AM, Carlo De Falco wrote:

> So, if it is now accepted to require a C++14 compatible compiler we
> should just document that and change the current configure test which
> looks for C++11 to look for C++14 instead.
> Otherwise, the use of "auto" for parameters should be abandoned. I'm not
> following in detail the discussion on this between jwe and Rik but it
> seems they are planning some way to work around this ...

We might still decide to require C++14 features but I also pushed the
following changeset which avoids auto in the lambda expressions and
moves the captured variables from the unwind_action objects to the
lambda expressions where possible.

   http://hg.savannah.gnu.org/hgweb/octave/rev/445e5ac1f58d

In most cases, the need for "auto" goes away when the variables are
captured in the lambda expression instead of being passed as arguments
to the function generated by the lambda expression.  So I meant to make
that change first, followed by eliminating the remaining uses of "auto"
in a separate changeset but forgot to split the change before pushing.

I also pushed a separate change to use the [=] capture default
specification where possible:

   http://hg.savannah.gnu.org/hgweb/octave/rev/1ac5a76ae91d

If we still decide against using the [=] capture default for style
reasons, it is easy to backout the changeset.

Please let me know if there are still problems with clang after these
changes.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: lambda expressions (was: Re: error with d28016d16e9a ...)

Rik-4
In reply to this post by John W. Eaton
On 10/02/2020 05:23 AM, John W. Eaton wrote:
On 10/1/20 8:13 PM, Rik wrote:
On 10/01/2020 12:42 PM, John W. Eaton wrote:

We could also decide to always use "[=]" when we need to capture something.  Then all variables that are needed, including "this" will be captured using the default rules for capture by value.

I don't think this is recommended practice.  The temporary anonymous struct that is created to represent the lambda expression will then have an argument in the constructor for every existing variable in the surrounding function.  The compiler *might* then optimize out all of the additional unused captures, but I think it would be better to just capture what you need either by value "[=variable_name]" if small like a built-in type or a pointer or by reference "[&variable_name]" if it is something large like the instance of a class.  For reference, I was using this https://dzone.com/articles/all-about-lambda-functions-in-cfrom-c11-to-c17.

As I understand it, the lambda expression only captures variables that are used.  Using a capture default specification actually seems better to me because the variables are already listed in the lambda expression, so explicitly listing them again is redundant.  To me, this seems quite similar to using "auto" to avoid writing out something that the compiler can figure out.  I don't know why the article you linked lists [=] and [&] as not recommended.  I didn't see an explanation and I'm not finding other similar recommendations.

jwe,

Just to fully close this issue, I created a function with a 1000 local variables and a lambda expression that used one of the variables.  I defined the capture expression as either "[=]" or "[=var995]" which captures just one variable.  I then compiled with gcc and compared the resulting binaries.  At least with gcc, there is no particular binary difference between using either expression which indicates it is safe to capture all variables as the compiler is smart enough to optimize out anything that is unused.  So, I think we're fine with adopting the conventions you proposed.

--Rik
Reply | Threaded
Open this post in threaded view
|

Re: error with d28016d16e9a (change to jsonencode.cc)

Octave - Maintainers mailing list
In reply to this post by Octave - Maintainers mailing list
On Oct 1, 2020, at 2:22 PM, Benjamin Abbott <[hidden email]> wrote:

On Oct 1, 2020, at 10:43 AM, Carlo De Falco <[hidden email]> wrote:


Il giorno 1 ott 2020, alle ore 17:26, Benjamin Abbott via Octave-maintainers <[hidden email]> ha scritto:

I’m seeing a build problem on the default branch when building on macOS (10.15.6) using Homebrew for dependencies and Apple’s clang version 12.0.0 (clang-1200.0.32.2).

does it work if you try building with CXXFLAGS="-std=c++14 "?

c.

Hi Carlo,

Your suggestion worked for me.

Ben

Hi John,

Apologies for the delay. I have no problem building on macOS.

Ben