patchGNU Octave - Patches: patch #9030, dim_vector constructor for any...

 
 

patch #9030: dim_vector constructor for any number of dimensions

Submitter:  Carnë Draug <carandraug>
Submitted:  Mon 13 Jun 2016 02:07:09 AM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 12 Aug 2016 03:16:32 PM UTC, comment #9: 
Carnë Draug <carandraug>
Group Member
Fri 12 Aug 2016 03:05:19 PM UTC, comment #8: 

The fix of comment #7 is perfect! Please commit it.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 11 Aug 2016 09:12:49 PM UTC, comment #7: 

Seems like I misunderstood what we will be able to do in C++14. So without planning ahead, I think that the new commit is cleaner and have exactly the same effect to fix your issue

https://bitbucket.org/carandraug/octave/commits/branch/dim-vector-ctor

Carnë Draug <carandraug>
Group Member
Thu 11 Aug 2016 05:26:30 PM UTC, comment #6: 

Yes thanks, your fix from comment #5 seems to fix the issue. I still get some errors, but these are "standards" I can fix myself and don't concern dim_vector.

I try to fix the remaining issues tomorrow. Then you can push these changes to the Octave repo.


libinterp/octave-value/ov-classdef.cc:1655:75: error: no matching function for call to ‘max(octave_idx_type, int)’
           Array<idx_vector> iv (dim_vector (1, std::max (ival.length (), 2)));


Kai Torben Ohlhus <siko1056>
Group Member
Thu 11 Aug 2016 03:44:17 PM UTC, comment #5: 
Carnë Draug <carandraug>
Group Member
Thu 11 Aug 2016 02:55:39 PM UTC, comment #4: 

Splitting the loop seems to work for me. Does this not work for you (or maybe I understood your problem incorrectly)?


#include <initializer_list>

class foo
{
public:
  template <typename... Ints>
  foo (const long int r, const long int c, Ints... lengths)
    :rep (new long int [2+sizeof... (Ints)])
  {
    *rep++ = r;
    *rep++ = c;
    for (const long int l: {lengths...})
      *rep++ = l;
    rep -= (2+sizeof... (Ints));
  }
  ~foo () { delete[] rep; }

  long int* rep;
};

int main() {
  int a = 5;
  int b = 6;
  int c = 7;
  foo f (a, b, c);
  return 0;
}


Just an aside: my preferred way to handle this would be a constructor that takes an initializer list instead of a parameter pack. The problem with that is that a user could then specify an initializer list with only one value so we'd need a size check in the constructor. Not only is that an extra check, it also means the constructor could throw. There should be no need for that because size of an initializer list is known at compile time. Indeed, c++14 specified it as constexpr so we will be able to use static_assert and a single initializer_list when we require c++14.

Carnë Draug <carandraug>
Group Member
Thu 11 Aug 2016 09:39:38 AM UTC, comment #3: 

Sorry I didn't try my 64-bit build earlier (due to vacation), but I am facing a serious problem, if octave_idx_type is not int (in my case long int).

I totally see through your simple nice code, but now in the for-loop in line 207 of dim_vector.h produces errors like this:


  CXX      libinterp/dldfcn/libinterp_dldfcn___delaunayn___la-__delaunayn__.lo
In file included from ./liboctave/array/Array.h:37:0,
                 from libinterp/corefcn/Cell.h:31,
                 from libinterp/dldfcn/__delaunayn__.cc:50:
./liboctave/array/dim-vector.h: In instantiation of ‘dim_vector::dim_vector(octave_idx_type, octave_idx_type, Ints ...) [with Ints = {int}; octave_idx_type = long int]’:
./libinterp/octave-value/ov-typeinfo.h:203:85:   required from here
./liboctave/array/dim-vector.h:207:5: error: unable to deduce ‘std::initializer_list<auto>&&’ from ‘{r, c, lengths#0}’
     for (const auto l: {r, c, lengths...})
     ^~~
./liboctave/array/dim-vector.h:207:5: note:   deduced conflicting types for parameter ‘auto’ (‘long int’ and ‘int’)


the type of auto cannot be resolved properly. Do you know an easy fix for this? I googled a lot, tried replacing auto by "octave_idx_type", split the loop, const_cast<>() but nothing seems to work.

Kai Torben Ohlhus <siko1056>
Group Member
Thu 07 Jul 2016 11:07:12 AM UTC, comment #2: 
Carnë Draug <carandraug>
Group Member
Sat 18 Jun 2016 05:47:02 PM UTC, comment #1: 

Speaking with Jordi on IRC, we came up with another patch that makes use of a variadic template to completely replaces all the existing macros.

(file #37523)

Carnë Draug <carandraug>
Group Member
Mon 13 Jun 2016 02:07:09 AM UTC, original submission:  

The attached patch adds a new constructor to dim_vector so it can work with any number of dimensions. This uses C++11 features. This can replace the existing macros that are limited to 7 dimensions.

It has the following issues:

  • throws an exception if less than 2 dimensions are defined. Maybe we should we have liboctave exceptions instead?


  • it's not container agnostic. Would be nice if it worked for vector but I'm not sure about the best way of doing that. Using a template for the STL sequence containers creates ambiguous constructors for Arrays.
Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37523:  dim-vector-variadic_template.cset added by carandraug (5KiB - application/octet-stream)
file #37457:  dim-vector-init_list.cset added by carandraug (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-07 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2016-07-07 lachlan CategoryNone Core : other
    2016-06-18 carandraug Attached File- Added dim-vector-variadic_template.cset, #37523
    2016-06-13 carandraug Attached File- Added dim-vector-init_list.cset, #37457

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code