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 OCTINTERP_API
octave_fields
{
  class fields_rep : public std::map<std::string, octave_idx_type>
  {
  public:
    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;

  private:
    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.

I was going to create a new topic but Discourse told me it was very similar to this thread, so I’ll post here:

While looking up the code in gcd.cc I found an old comment referring to “C++0x” in the future tense. There are two other locations in the codebase with that reference, one in error.cc and one in call-stack.cc (pasted below). Since it has been 11+ years since C++11 was called “C++0x”, and this thread indicates some changes were made already, are these comments still relevant?

$ grep -RiC2 "C++0x" lib*
libinterp/corefcn/call-stack.cc-
libinterp/corefcn/call-stack.cc-  // Use static fields for the best efficiency.
libinterp/corefcn/call-stack.cc:  // NOTE: C++0x will allow these two to be merged into one.
libinterp/corefcn/call-stack.cc-  static const char *bt_fieldnames[] =
libinterp/corefcn/call-stack.cc-    { "file", "name", "line", "column", nullptr };
--
libinterp/corefcn/error.cc-
libinterp/corefcn/error.cc-  // Use static fields for the best efficiency.
libinterp/corefcn/error.cc:  // NOTE: C++0x will allow these two to be merged into one.
libinterp/corefcn/error.cc-  static const char *bt_fieldnames[] =
libinterp/corefcn/error.cc-    { "file", "name", "line", "column", nullptr };
--
libinterp/corefcn/gcd.cc-// Don't use the Complex and FloatComplex typedefs because we need to
libinterp/corefcn/gcd.cc-// refer to the actual float precision FP in the body (and when gcc
libinterp/corefcn/gcd.cc:// implements template aliases from C++0x, can do a small fix here).
libinterp/corefcn/gcd.cc-template <typename FP>
libinterp/corefcn/gcd.cc-static void

I never could figure out how to simplify the first two instances (with bt_fieldnames). I would just leave them as is.

There might be a simplification in gcd.cc, but it probably isn’t worth spending time on since the code works, it is only in one file, and it isn’t going to lead to a performance improvement (maybe just a readability improvement).

Maybe something like the attached change could be a starting point for the field names initialization issue? I worked on this patch some time ago but forgot about it.

init-list-diffs.txt (3.5 KB)

I think the comment in gcd.cc can just be removed and the code left as it is.

Or maybe the comment is there because the idea was to write something like

// CT is assumed to be a complex value type.
template <typename CT>
static void
divide (const CT& a, const CT& b, CT& q, CT& r)
{
  CT::value_type qr = std::floor ((a/b).real () + 0.5);
  CT::value_type qi = std::floor ((a/b).imag () + 0.5);

  q = CT (qr, qi);

  r = a - q*b;
}

That comment might be there because it was written before the std::complex<T>::value_type typedef existed or maybe the person who wrote the comment didn’t know about it that it was provided for cases like this one.

I have removed that comment in gcd.cc on stable: octave: ed06f74e16cf

1 Like