Advice for final patch submission from Github mirror

Continuing the discussion from GSoC 2022: Project-ode15{i,s}:

Hello @gsoc_mentors ,
I have been working on the GSoC 2022: Project-ode15{i,s}, and have consolidated the work so far into this patch :
0001-add-support-for-sparse-computations-in-ode15-i-s-usi.patch (60.0 KB) as discussed with my mentor @cdf . I have provided some more insights into what should happen to this code further in the conversation above.

The commit here is to the current default of my fork of the GNU Octave Github mirror, namely this . I had hoped for @cdf to review this patch once before adding it to the patch tracker.

There are multiple commits in the commit history of the various branches in my forked repository of the mirror, and while they all contain meaningful commit messages, quite a lot of them are now rendered useless as they were for function-wise tests or sometimes me committing files generated by ./bootstrap to the forked repository. Hence, for the sake of convenience and ability of the final patch to be applied, the final commit contains no history of this log. I looked at few past gsoc patches as well, and did not seem to find the associated changelogs. However, since they were done on bitbucket repositories that I am now unable to access, whereas my commit id is from github, I just wanted to make sure I am not doing something wrong ad get this reviewed.

Hi @sarrah-basta here is your patch in the form of a mercurial changeset

final_patch_gsoc_ode15i_2022 (59.3 KB)

this seems to apply cleanly for me on the default branch so you can just go ahead and upload it to the patch tracker so that it does not get lost.

As we discussed previously it will probably take much longer before this can be actually pushed in Octave core, so it would be nice if you could stay around to help with the further cleanup even after CSOC.

3 Likes

Hello Mentors @cdf ,

Following this patch that I submitted on the patch tracker at this link, I looked over my code and have tried to make changes to it such that it should now follow the Octave C++ Style Guide. While this was easy and almost already done for the __ode15__.cc file, for the two new files I have added namely oct-sundials.h and oct-sundials.cc, being wrappers to Octave functions for SUNDIALS, the teamplate I followed for them was by SUNDIALS as well, which is also written in C. Hence some things that I haven’t been able to follow from the Style Guide are :

  1. Header files should not use any “#if defined (HAVE_FEATURE)” conditionals. This is not quite true yet, but we are almost there. No new conditionals may be added.
    As my wrappers need to be consistent with versions of SUNDIALS both before and after version 6.0.0, where they introduced the SUNContext object, and I am writing declarations of functions that use the object, I had to use #if defined (HAVE_SUNDIALS_SUNCONTEXT) to be able to do this properly.
  2. Use references when passing variables that will be changed by a subroutine rather than the C-style method of passing pointers.
    Once again, with SUNDIALS being written in C, and the wrappers being pointers to functions defined by them, I used the same template as was expected, and hence sometimes do pass pointers when passing variables. I am not sure if this should/could be changed.
  3. There are quite a few comments in these files, but they are not yet in the form or have the content to be doxygen-compatible. If we are interested in these wrapper also having a doxygen interface for the comments, I can work on that documentation.

Apart from this, the changes I have made are recorded in this commit . Please have a look and tell me if anything more can be done to make the code better and more readable, or if I have missed any points in the style guide. If approved I can create a patch merging this with the earlier changes and upload it to the tracker as well.