C++11 simplification of static initialization

In call-stack.cc I found this piece of code and note:

  // Use static fields for the best efficiency.
  // NOTE: C++0x will allow these two to be merged into one.
  static const char *bt_fieldnames[] =
    { "file", "name", "line", "column", nullptr };

  static const octave_fields bt_fields (bt_fieldnames);

Octave has moved beyond C++0x and now allows C++11 standards. So is there a way to combine these two lines so there is only one static initialization of bt_fields?

Memory is basically free these days so it’s not horrible that we maintain a few extra C strings for the life of the Octave program in the variable bt_fieldnames. But if it were trivial to get rid of it that would be nice.

Looking at the datatype octave_fields

// A class holding a map field->index.  Supports reference-counting.
  class fields_rep : public std::map<std::string, octave_idx_type>
    fields_rep (void) : std::map<std::string, octave_idx_type> (), m_count (1) { }
    fields_rep (const fields_rep& other)
      : std::map<std::string, octave_idx_type> (other), m_count (1) { }

    octave::refcount<octave_idx_type> m_count;

    fields_rep& operator = (const fields_rep&); // no assignment!

  fields_rep *m_rep;

  static fields_rep *nil_rep (void);

// ...

the called constructor is

octave_fields::octave_fields (const char * const *fields)
  : m_rep (new fields_rep)
  octave_idx_type n = 0;
  while (*fields)
    (*m_rep)[std::string (*fields++)] = n++;

Thus, maybe what the author of this comment wants to say is that the value

{ "file", "name", "line", "column", nullptr }

can be passed as Rvalue and does not be assigned to the Lvalue bt_fieldnames explicitly? However, I think an initializer lists constructor needs to look a little different, but is for sure preferable.

Maybe something like this change?

init-list-diffs.txt (1.5 KB)

[Edit:] I also looked at allowing octave_fields to be constructed from any container that behaves like a standard sequence container, but that will require more work because of the way we currently use our custom string_vector object. Which reminds me that we might want to replace that and other uses of Array objects with standard containers where possible. And that maybe the octave_fields object should be a hidden detail of Octave’s map and scalar_map objects.

I think your code will work. It also reminds me why people choose scripting languages like Python and Perl over compiled languages like C and C++. The amount of overhead to accomplish this relatively simple optimization is just so high including declaring a new constructor in a header file, writing its implementation in the associated .cpp file, and then finally using the new constructor back in the original code.

One question, would it be better to add another constructor for string_vector and then have octave_fields just use the existing constructor that takes a const string_vector& ? It seems like string_vector is a more fundamental class and if we’re going to add a constructor maybe it makes sense to do it there.

Also, the code for the existing constructor which takes char * is

octave_fields::octave_fields (const char * const *fields)
  : m_rep (new fields_rep)
  octave_idx_type n = 0;
  while (*fields)
    (*m_rep)[std::string (*fields++)] = n++;

while the proposed code is

+  for (const auto& field : fields)
+    (*m_rep)[field] = n++;

This made me think about the call to std::string. Do we want what amounts to an explicit type cast or should we let the compiler handle this decision and just write

    (*m_rep)[*fields++] = n++;

This has been on my list. Let’s have some discussion on this at the Octave developer’s meeting today.

Agreed. I looked at this problem last week and gave up on it because I headed off on the wrong path and couldn’t understand what to do to make things work the way I wanted and expected they should.

You are right that we don’t need an explicit call to the std::string constructor there but I’m pretty sure that the std::string constructor will be called anyway, because the operator[] for the underlying std::map<std::string, octave_idx_type> object only accepts std::string as an argument. There is no special case for const char* as an argument.

We might be able to make things work reasonably well and allow the map constructor to accept standard sequence containers if we eliminate the octave_fields object from the public interface of the map classes or, if we can’t do that, by making the octave_fields constructors explicit to avoid conflicts caused by automatic type conversion.