comment #28:

> What is the exact error with the changes from comment #24 and comment #26? Could you show the exact changes that you applied (as a patch or `verbatim` block)? Could you please attach the generated file "libinterp/dldfcn/module.mk" (in your source tree) after you applied those changes?

The reason for the problem arising last time was probably something different, there was some cache which was preventing installation of a package and causing some error.

I tried again with the following changes as suggested by you and it worked. Thanks a lot !

diff --git a/libinterp/dldfcn/config-module.awk b/libinterp/dldfcn/config-module.awk
index 1a2ea9cc8b..0ab680ef4c 100644
--- a/libinterp/dldfcn/config-module.awk
+++ b/libinterp/dldfcn/config-module.awk
@@ -22,11 +22,11 @@
 ## <https://www.gnu.org/licenses/>.
 ##
 ########################################################################
-
+
 BEGIN {
   FS = "|";
   nfiles = 0;
-
+
   print "## DO NOT EDIT -- generated from module-files by config-module.awk";
   print ""
   print "EXTRA_DIST += \\"
@@ -45,15 +45,14 @@ BEGIN {
   libraries[nfiles] = $4;
 } END {
   sep = " \\\n";
-  print "DLDFCN_SRC = \\";
+  printf ("DLDFCN_SRC =");
   for (i = 1; i <= nfiles; i++) {
-    if (i == nfiles)
-      sep = "\n";
-    printf ("  %%reldir%%/%s%s", files[i], sep);
+    split (files[i], src_files, " ");
+    printf ("%s  %%reldir%%/%s", sep, src_files[1]);
   }
   print "";
-
-  sep = " \\\n";
+
+  print "";
   print "DLDFCN_LIBS = $(DLDFCN_SRC:.cc=.la)";
   print "";
   print "octlib_LTLIBRARIES += $(DLDFCN_LIBS)";
@@ -63,21 +62,16 @@ BEGIN {
   print "";
   print "%.oct : %.la"
   print "\t$(AM_V_GEN)$(INSTALL_PROGRAM) %reldir%/.libs/$(shell $(SED) -n -e \"s/dlname='\\([^']*\\)'/\\1/p\" < $<) $@"
-
+
   for (i = 1; i <= nfiles; i++) {
-    basename = files[i];
+    split (files[i], src_files, " ");
+    basename = src_files[1];
     sub (/\.cc$/, "", basename);
     print "";
-    if (files[i] == "__ode15__.cc")
-    {
-      printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/__ode15__.cc %%reldir%%/oct-sundials.cc \n",
-            basename);
-    }
-    else
-    {
-      printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/%s\n",
-            basename, files[i]);
-    }
+    printf ("%%canon_reldir%%_%s_la_SOURCES =", basename);
+    for (j in src_files)
+      printf (" %%reldir%%/%s", src_files[j]);
+    print "";
     if (cppflags[i])
       {
         printf ("%%canon_reldir%%_%s_la_CPPFLAGS = $(libinterp_liboctinterp_la_CPPFLAGS) %s\n",
@@ -90,15 +84,21 @@ BEGIN {
     printf ("%%canon_reldir%%_%s_la_DEPENDENCIES = $(OCT_LINK_DEPS)\n",
             basename);
   }
-
+
   print "";
   print "$(srcdir)/%reldir%/module.mk: $(srcdir)/%reldir%/config-module.sh $(srcdir)/%reldir%/config-module.awk $(srcdir)/%reldir%/module-files";
   print "\t$(AM_V_GEN)$(SHELL) $(srcdir)/%reldir%/config-module.sh $(srcdir)";
-
+
   print "";
   print "DLDFCN_OCT_FILES = $(DLDFCN_LIBS:.la=.oct)";
   print "";
-  print "DLDFCN_DEFUN_FILES = $(DLDFCN_SRC)";
+  printf ("DLDFCN_DEFUN_FILES =");
+  for (i = 1; i <= nfiles; i++) {
+    split (files[i], src_files, " ");
+    for (j in src_files)
+      printf ("%s  %%reldir%%/%s", sep, src_files[j]);
+  }
+  print "";
   print "";
   print "DLDFCN_PKG_ADD_FILE = %reldir%/PKG_ADD";
   print "";
@@ -123,7 +123,7 @@ BEGIN {
   print " $(DLDFCN_OCT_FILES)";
   print "";
   print "DIRSTAMP_FILES += %reldir%/$(octave_dirstamp)";
-
+
   print "";
   print "libinterp_CLEANFILES += \\";
   print "  $(DLDFCN_PKG_ADD_FILE) \\";
diff --git a/libinterp/dldfcn/module-files b/libinterp/dldfcn/module-files
index da6589c09f..b21ac6f7c0 100644
--- a/libinterp/dldfcn/module-files
+++ b/libinterp/dldfcn/module-files
@@ -4,7 +4,7 @@ __fltk_uigetfile__.cc|$(FLTK_CPPFLAGS) $(FT2_CPPFLAGS)|$(FLTK_LDFLAGS) $(FT2_LDF
 __glpk__.cc|$(GLPK_CPPFLAGS)|$(GLPK_LDFLAGS)|$(GLPK_LIBS)
 __init_fltk__.cc|$(FLTK_CPPFLAGS) $(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)|$(FLTK_LDFLAGS) $(FT2_LDFLAGS)|$(FLTK_LIBS) $(FT2_LIBS) $(OPENGL_LIBS)
 __init_gnuplot__.cc|$(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)||
-__ode15__.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
+__ode15__.cc oct-sundials.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
 __voronoi__.cc|$(QHULL_CPPFLAGS)|$(QHULL_LDFLAGS)|$(QHULL_LIBS)
 audiodevinfo.cc|$(PORTAUDIO_CPPFLAGS)|$(PORTAUDIO_LDFLAGS)|$(PORTAUDIO_LIBS)
 audioread.cc|$(SNDFILE_CPPFLAGS)|$(SNDFILE_LDFLAGS)|$(SNDFILE_LIBS)

I added some comments to that commit on GitHub. (Actually twice because the first round of comments disappeared into somewhere I can't find...)

Hello @mmuetzel, thank you for all your comments.

As the rebase caused quite some changes, as changes to the coding style were made to the `__ode15__.cc` file which conflicted with my own changes, it took me some time and a lot of commits to figure that out. Since the current changes were spread over multiple commits, and seeing your comments you too had some difficulty understanding how they were structured, I have fixed all the whitespace and unwanted changes and squashed all my commits into one with a clean code diff for you to review, situated at
https://github.com/sarrah-basta/octave/tree/new-gsoc-final-aptch .

This contains all changes except the following:

comment #26:

> These changes to `config-module.awk` will hopefully take care of the new file :

I tried testing this, and while it kind of works, meaning the code for the implementation I have written, containing Octave Wrappers for IDA, works correctly, i.e the oct-sundials.* files are being compiled and used correctly, it runs into an error related to SUNContext (defined as a configure flag) in the KLU part (which was supprted prior to my project). I will try figuring out the exact problem and fix the script so that the files are not hardcoded soon.

Try using this alternative change additionally to the change from comment #24:

diff -r 96ed8d1d0c59 libinterp/dldfcn/config-module.awk
--- a/libinterp/dldfcn/config-module.awk        Sat Mar 04 00:59:48 2023 -0500
+++ b/libinterp/dldfcn/config-module.awk        Tue Mar 07 16:12:21 2023 +0100
@@ -49,7 +49,8 @@
   for (i = 1; i <= nfiles; i++) {
     if (i == nfiles)
       sep = "\n";
-    printf ("  %%reldir%%/%s%s", files[i], sep);
+    split (files[i], src_files, " ");
+    printf ("  %%reldir%%/%s%s", src_files[1], sep);
   }
   print "";

@@ -65,11 +66,14 @@
   print "\t$(AM_V_GEN)$(INSTALL_PROGRAM) %reldir%/.libs/$(shell $(SED) -n -e \"s/dlname='\\([^']*\\)'/\\1/p\" < $<) $@"

   for (i = 1; i <= nfiles; i++) {
-    basename = files[i];
+    split (files[i], src_files, " ");
+    basename = src_files[1];
     sub (/\.cc$/, "", basename);
     print "";
-    printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/%s\n",
-            basename, files[i]);
+    printf ("%%canon_reldir%%_%s_la_SOURCES =", basename);
+    for (j in src_files)
+      printf (" %%reldir%%/%s", src_files[j]);
+    print "";
     if (cppflags[i])
       {
         printf ("%%canon_reldir%%_%s_la_CPPFLAGS = $(libinterp_liboctinterp_la_CPPFLAGS) %s\n",

It is completely untested. So, try to build with it before using it.

There might still be an issue with the new file missing from the distribution tarball. But I hope, it'd do the same that your hardcoded exception currently does.

I added a couple of comments on GitHub.

Something seems to have gone wrong when merging the whitespace changes in `__ode15__.cc`. It's difficult to review the changes in that file in it's current state.
The code between `OCTAVE_NAMESPACE_*` blocks is no longer indented. Could you please adapt for that style change?

comment #20:

> Another style remark:
> You often use the pattern `return(val);` or `return (val);` when returning a value from a function. For Octave code, you should use `return val;` (with a single space and no parenthesis).

Alright, I'll fix these soon too.
 
I am currently facing a slight issue in the operating system I work in, but I'll make all these changes at once and commit soon.

comment #17:

> IIUC, you could use the exact same names as the one in SUNDIALS if you were to move those functions to the octave namespace. The one from SUNDIALS could be called as `::example_function`. And the one from Octave could be called as `octave::example_function`. Those would be different identifiers.
>

I think when something similar had come up during talking to Carlo as well, and some of the functions have their names based on SUNDIALS native types. And iirc, the consensus then was we shouldn't have something called a SUNSparseMatrix() as a function in Octave. While some of them don't have this and can be called by their exact name as you said example octave::N_VScaleAddMulti ...

> If we were also to change the Naming Style from CamelCase,I believe they could be moved to `namespace octave` as our names wouldnt clash with the defualt implementations of those functions in Sundials as well.

IIUC, you could use the exact same names as the one in SUNDIALS if you were to move those functions to the octave namespace. The one from SUNDIALS could be called as `::example_function`. And the one from Octave could be called as `octave::example_function`. Those would be different identifiers.

But I'll leave that up to you to decide.

comment #11:

> The current naming convention is a bit confusing. With some functions starting with `OCT` or `_OCT` and some ending with `_Octave`.
> If those function have 1-to-1 correspondents in SUNDIALS, you could use that name with their conventions imho. But, I'd vote for moving them to the `namespace octave`. Use the macros `OCTAVE_BEGIN_NAMESPACE(octave)` and `OCTAVE_END_NAMESPACE(octave)` for that.

The current conventions were just based on how existing implementations in SUNDIALS using other libraries differentiated their wrappers from the default implemented functions, such as this implementation of a vector using OpenMP https://sundials.readthedocs.io/en/v6.2.0/nvectors/NVector_links.html#the-nvector-openmp-module . And for generic type names in SUNDIALS, Carlo and I had just decided to replace the `SUN` with `OCT`. Hence the confusion ...

If we were also to change the Naming Style from CamelCase,I believe they could be moved to `namespace octave` as our names wouldnt clash with the defualt implementations of those functions in Sundials as well.

@sarrah: Which branch/repository are you working on? I don't see any recent changes in your GitHub clone after last September.

The current naming convention is a bit confusing. With some functions starting with `OCT` or `_OCT` and some ending with `_Octave`.
If those function have 1-to-1 correspondents in SUNDIALS, you could use that name with their conventions imho. But, I'd vote for moving them to the `namespace octave`. Use the macros `OCTAVE_BEGIN_NAMESPACE(octave)` and `OCTAVE_END_NAMESPACE(octave)` for that.

Once you removed the `extern "C"`, also change over all uses of `NULL` to `nullptr`.

> Is that necessary?

Good question ...

It was definitely necessary during development because @sarrah initially tried using the Octave SparseMatrix class in an independent ida based application written in C.

From within Octave this may not be necessary anymore if ida only
accesses functions in the context via pointers ...

> Could you please point out where it is defined in the sources of SUNDIALS?

It's in the config file that is generated by CMake and installed
as include/sundials/sundials_config.h IIUC it is only needed on windows.

> Or alternatively (and maybe preferably), rebase on a current head of the default branch.

Yes, that would be great

comment #3:

> @mmuetzel thanks for looking into this.
>
>  - I don't have time right now to respond to all your comments, I hope maybe @sarrah can?

 
Yes, thank you @mmuetzel and @cdf for your comments and looking into this.


For @mmuuetzel 's comments :
comment #2:

> * I can't see where SUNDIALS_EXPORT is defined. What is it supposed to do?

As @cdf was kind to point out, SUNDIALS_EXPORT is a macro in the C API of SUNDIALS and not defined by me. It is used to expose the replacement functions created by me (which replace what would ideally be SUNDIALS functions) to the rest of the SUNDIALS API. 

> * CamelCase in function names is discouraged in Octave's sources. Use all lower case with underscores as separators (unless there is a good reason to deviate from that convention here).
> * Many functions have names that contain "OCT" or "_Octave". Would it make sense to move them to the `octave` namespace instead?

Regarding the above two points, as @cdf pointed out, most of these are with regards to using the SUNDIALS C API explained above, as those functions that are written in C++, still have to be called by the  C API, and are hence exposed by `#extern C`. Hence I too do not think using such C++ functionality will be possible.

> * Remove the include guard `__CONFIG_H_INCLUDE_GUARD` from `__ode15__.cc`. `.cc` files aren't supposed to be included in other files normally. Is this different for this file?
> * Revert the white space changes in your patch for the pre-processor macros. The hash `#` is supposed to be in column 1. Spaces for indentation follow after the `#`. Use two spaces for indentation.
> * Avoid using old-style cast in NV_CONTENT_C. Use `reinterpret_cast` instead. Or does that not work for some reason here?
> * Use a space between function name and following opening parenthesis.

 
With regards to these points, this patch is a slightly older version, and I did make some changes to the code after this especially to accommodate the Coding Style. These are stored on the git repository mentioned by @cdf and the changes are much clearly discussed by me at : https://octave.discourse.group/t/advice-for-final-patch-submission-from-github-mirror/3232/3?u=sarrah-basta

If you could have a look at these it would be great. I will myself try ensuring that I have followed all the style guide comments you posted about and try rebasing my code on a current head on the default branch of the git mirror and reply soon.

@mmuetzel thanks for looking into this.

 - I don't have time right now to respond to all your comments, I hope maybe @sarrah can?

 - from quickly reading your comments I guess you are not considering that @sarrah was using the C API of SUNDIALS so I'm not sure that using some C++ feature (such as e.g. namespaces)
is feasible ...

 - Also some naming conventions are consistent with the SUNDIALS
coding style rather than Octave's


 - The GIT repo used by @sarrah for her project is still available here : https://github.com/sarrah-basta/octave
I think this is the branch used for the final submission : https://github.com/sarrah-basta/octave/tree/gsoc-submission-final maybe one of the first things to do would be to merge default into that branch and see wether it still works?

Here are a few additional relevant links associated with this project :

Discussion with the Community : https://octave.discourse.group/t/gsoc-2022-project-ode15-i-s/1343?u=sarrah-basta

Progress documentation throughout the Project : https://sarrah-basta.github.io/blogs/

Final Project Report for GSoC Submission : https://sarrah-basta.github.io/blogs/2022/09/11/GSoC_'22_GNU_Octave_Project_Report.html

Also attaching an updated version (v2) of the patch for the project.

(file #53687)