bugGNU Octave - Bugs: bug #61784, builtins deprecated warning problem

 
 

bug #61784: builtins deprecated warning problem

Submitted by:  None
Submitted on:  Mon 10 Jan 2022 09:29:15 AM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  7.0.90
Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 21 Jan 2022 03:46:50 PM UTC, comment #12: 

Thanks!

Tested with ac74380e9d46 on Windows and the function from comment #10 compiles with only one warning for Fdisp here, too.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Tue 18 Jan 2022 06:30:48 PM UTC, comment #11: 

I pushed the following changeset.  Marking as ready for test.

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

John W. Eaton <jwe>
Project Administrator
Tue 18 Jan 2022 06:03:07 PM UTC, comment #10: 

Oh, I see.  I forgot to attach the earlier change but I guess it is not needed.  Instead, try the patch attached with this comment.  With it, the following version of lpath.cc compiles for me with just one deprecated function warning for Fdisp.

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

DEFUN_DLD (lpath, args, nargout,
           "Hello World Help String")
{
  octave::interpreter& interp = *octave::interpreter::the_interpreter ();

  Fdisp (octave_value ("hello"));
  F__dump_load_path__ (interp);

  octave::Fdisp (octave_value ("hello"));
  octave::F__dump_load_path__ (interp);

  return octave_value_list ();
}

(file #52678)

John W. Eaton <jwe>
Project Administrator
Tue 18 Jan 2022 05:50:08 PM UTC, comment #9: 

Sorry. I misunderstood where the warning was actually coming from.

Looking at this again: Maybe, we don't even need to deprecate these functions outside the namespace. Argument dependent lookup (ADL) will automatically detach to the function in the `octave` namespace (because the `interpreter` object is in that namespace as well).
Users of these functions wouldn't need to do anything in their code if we removed these functions entirely. There's not really a need for a deprecation warning.

That is probably what the original reporter tried to explain the entire time. (But I didn't understand until now. Sorry!)

I now agree with the original reporter to just remove the deprecated functions that take an interpreter argument from that header.

Markus Mützel <mmuetzel>
Project Member
Tue 18 Jan 2022 05:17:40 PM UTC, comment #8: 

@Markus:  I tried the attached change so that the template instantiation and pointer trick is applied to all functions declared in builtin-defun-decls.h that have octave::interperter& as the first argument.  Then I tried compiling the following .oct file function:

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

DEFUN_DLD (lpath, args, nargout,
           "Hello World Help String")
{
  octave::interpreter& interp = *octave::interpreter::the_interpreter ();

  Fdisp (octave_value ("hello"));
  F__dump_load_path__ (interp);

  return octave_value_list ();
}

With that, it does compile and I correctly see one deprecated function warning about Fdisp.  But I also see deprecated function warnings for all the functions in builtin-defun-decls.h that have octave::interpreter& as the first argument, not just the one that is used.  I couldn't see a way to avoid that.

John W. Eaton <jwe>
Project Administrator
Tue 18 Jan 2022 02:17:05 PM UTC, comment #7: 

How do we want to move forward with this?

Is it ok to have those function templates with their deprecated instantiation?

I found this old (resolved) bug report about deprecating function templates:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33911

Apparently, the current behavior is by design? Even the weird behavior of not showing a deprecation warning unless a template is instantiated at least twice? wth???

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 02:41:03 PM UTC, comment #6: 

I opened bug #61812 for the issue with M_PI in a header.

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 09:35:15 AM UTC, comment #5: 

I played around with this a while. It looks like function templates can't be deprecated. Template functions can. But the warning is only shown if they are used at least twice(?).

The following change in `builtin-defun-decls.h` makes the deprecation warning show for me. But only if the wrapper template is creating a pointer to that function first. Is this a compiler bug?

template <typename T>
octave_value_list
F__dump_load_path__ (T& interp, const octave_value_list& args = octave_value_list (), int nargout = 0)
{
  octave_value_list (*f_ptr) (T&, const octave_value_list&, int);
  f_ptr = &F__dump_load_path__<T>;

  return octave::F__dump_load_path__ (interp, args, nargout);
}

template
octave_value_list
OCTAVE_DEPRECATED (7, "use 'octave::F__dump_load_path__' instead")
F__dump_load_path__<octave::interpreter> (octave::interpreter& interp, const octave_value_list& args = octave_value_list (), int nargout = 0);

While playing around with this, I noticed the following:

>> mkoctfile -std=c++11 lpath.cc
[...]
d:\svn\octave\test\octave-2022-01-11-20-55-w64 of-general\octave-2022-01-11-20-55-w64\mingw64\include\octave-8.0.0\octave\oct-cmplx.h: In function 'bool operator>=(T, const std::complex<_Tp>&)':
d:\svn\octave\test\octave-2022-01-11-20-55-w64 of-general\octave-2022-01-11-20-55-w64\mingw64\include\octave-8.0.0\octave\oct-cmplx.h:99:39: error: 'M_PI' was not declared in this scope
   99 |            if (by == static_cast<T> (-M_PI))                                 \
      |                                       ^~~~
d:\svn\octave\test\octave-2022-01-11-20-55-w64 of-general\octave-2022-01-11-20-55-w64\mingw64\include\octave-8.0.0\octave\oct-cmplx.h:236:1: note: in expansion of macro 'DEF_COMPLEXR_COMP'
  236 | DEF_COMPLEXR_COMP (>=, >)
      | ^~~~~~~~~~~~~~~~~
[...]
error: mkoctfile: building exited with failure status

It only compiles if I enable GNU extensions (e.g. -std=gnu++14).
Should we support compiling without GNU extensions?

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 06:02:58 AM UTC, comment #4: 

The intent was to move all of these built-in functions inside the octave namespace while also providing inline functions that would also make them visible in the global namespace.

We want to provide the old names to allow for a transition period of at least two major releases so removing the old names in version 7 is not really an option.

Using a template (or some other method that works to avoid the ambiguity) could be OK.  However, when I changed the inline function in builtin-defun-decls.h to be

template <typename T>
OCTAVE_DEPRECATED (7, "use 'octave::F__dump_load_path__' instead")
octave_value_list
F__dump_load_path__ (T& interp, const octave_value_list& args = octave_value_list (), int nargout = 0)
{
  return octave::F__dump_load_path__ (interp, args, nargout);
}

the warning about the deprecated function name also disappeared.  Am I doing something wrong?  It's not particularly useful to lose the warning since we would like users to be alerted to this namespace change.

John W. Eaton <jwe>
Project Administrator
Wed 12 Jan 2022 05:41:23 PM UTC, comment #3: 

Aah, thanks for the updated reproducer.
I can confirm this issue. The error for me is:

>> mkoctfile lpath.cc
lpath.cc: In function 'octave_value_list Flpath(const octave_value_list&, int)':
lpath.cc:12:20: error: call of overloaded 'F__dump_load_path__(octave::interpreter&)' is ambiguous
   12 | F__dump_load_path__(interp);
      | ~~~~~~~~~~~~~~~~~~~^~~~~~~~
In file included from lpath.cc:3:
c:\progra~1\gnuoct~1\octave~1.90\mingw64\include\octave-7.0.90\octave\builtin-defun-decls.h:2530:1: note: candidate: 'octave_value_list F__dump_load_path__(octave::inte
rpreter&, const octave_value_list&, int)'
 2530 | F__dump_load_path__ (octave::interpreter& interp, const octave_value_list& args = octave_value_list (), int nargout = 0)
      | ^~~~~~~~~~~~~~~~~~~
In file included from lpath.cc:3:
c:\progra~1\gnuoct~1\octave~1.90\mingw64\include\octave-7.0.90\octave\builtin-defun-decls.h:53:1: note: candidate: 'octave_value_list octave::F__dump_load_path__(octave
::interpreter&, const octave_value_list&, int)'
   53 | F__dump_load_path__ (octave::interpreter&, const octave_value_list& = octave_value_list (), int = 0);
      | ^~~~~~~~~~~~~~~~~~~
error: mkoctfile: building exited with failure status

The error can be avoided, e.g., by moving the entire function into the octave namespace:

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

namespace octave
{
DEFUN_DLD (lpath, args, nargout,
"Hello World Help String")
{
octave::interpreter& interp = *octave::interpreter::the_interpreter ();

F__dump_load_path__(interp);

return octave_value_list ();
}
}

Or by explicitly calling the function in the octave namespace:

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

DEFUN_DLD (lpath, args, nargout,
"Hello World Help String")
{
octave::interpreter& interp = *octave::interpreter::the_interpreter ();

octave::F__dump_load_path__(interp);

return octave_value_list ();
}

But I'd guess the intention was that the deprecated function wrapper used in the original example should be working, too.

We probably don't want to remove the original functions (without namespace) without any grace period.
I don't know if we like to use the work-around with the template.

CC'ing jwe.

Markus Mützel <mmuetzel>
Project Member
Wed 12 Jan 2022 05:18:38 PM UTC, comment #2: 

Oops! Sorry for mistake. Actually there are more than 250 functions that have octave::interpreter as their first argument. I just intended to provide a simple example Ffdisp was the correct. However in the following example I used F__dump_load_path__ that is one of those 250 functions:

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

DEFUN_DLD (lpath, args, nargout,
           "Hello World Help String")
{
  octave::interpreter& interp = *octave::interpreter::the_interpreter ();

  F__dump_load_path__(interp);

  return octave_value_list ();
}

Anonymous
Wed 12 Jan 2022 04:05:28 PM UTC, comment #1: 

I don't see a declaration of `Fdisp` that takes an interpreter object as the first argument anywhere in `builtin-defun-decls.h`.
Should it be there? Why do you think it should? Is that the error that you are reporting?

Markus Mützel <mmuetzel>
Project Member
Mon 10 Jan 2022 09:29:15 AM UTC, original submission:  

There is a compilation failure for .oct files that are calling builtin functions via including "builtin-defun-decls.h".

Minimal example:

#include <octave/oct.h>
#include <octave/interpreter.h>
#include <octave/builtin-defun-decls.h>

DEFUN_DLD (hello, args, nargout,
           "Hello World Help String")
{
  octave::interpreter& interp = *octave::interpreter::the_interpreter ();

  Fdisp(interp, octave_value("hello"));

  return octave_value_list ();
}

It appears that it only has affected the builtins that have the following signature:

octave_value_list
octave::builtin_name (octave::interpreter&, const octave_value_list& = octave_value_list (), int = 0);

For each builtin an inline function is defined in that header that calls the correct function and is marked as deprecated like this :

inline octave_value_list
builtin_name (octave::interpreter& interp, const octave_value_list& args = octave_value_list (), int nargout = 0)
{
  return octave::builtin_name (interp, args, nargout);
}

I think because of the rules of the argument dependent lookup (ADL)[1] the compiler finds both overloads and cannot continue to decide to select the proper one.

Solution:

Two solutions to my understanding may be used:

1. Remove all deprecated warnings and newly defined inline functions and force the users to use the correct signature "octave::builtin_name". It also reduces the header size and speeds up the compilation time.

2. Using template:

template <typename T>
octave_value_list
builtin_name (T& interp, const octave_value_list& args = octave_value_list (), int nargout = 0)
{
  return octave::builtin_name (interp, args, nargout);
}

[1] https://en.cppreference.com/w/cpp/language/adl

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by mmuetzel (Posted a comment)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-21 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2022-01-18 jwe StatusConfirmed => Ready For Test
    2022-01-18 jwe Attached File- => Added builtin-defun-decls-diffs.txt, #52678
    2022-01-18 jwe Operating SystemMicrosoft Windows => Any
    2022-01-12 mmuetzel StatusNone => Confirmed
        Carbon-Copy- => Added jwe

    Back to the top


    Powered by Savane 3.7