octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

'auto' uses and for-range loops


From: Rik
Subject: 'auto' uses and for-range loops
Date: Sun, 12 Jun 2016 12:14:03 -0700

All,

I've checked in two csets that start to use the C++11 'auto' keyword to
improve the code.  In the process, I've started to develop some guidelines
which I thought I would share.

First, why bother with code that works?  Well, it might work, but it is
awfully hard to figure out what is going on, and this makes it hard to
maintain and/or add new features.  Below is an example from debug.cc

-- Code to search a list of function names
for (std::list<std::string>::const_iterator p = subfcn_names.begin ();
     p != subfcn_names.end (); p++)
  {
    std::map<std::string, octave_value>::const_iterator
      q = subfcns.find (*p);
-- End Code

C++ is never going to have scripting language syntax, but this is a mess of
scoping operators, template parameterizations, and pointer defererences.

It can be improved somewhat by using 'auto' to find the type of the variable p.

-- Code with 'auto'
for (auto p = subfcn_names.begin (); p != subfcn_names.end (); p++)
-- End Code

Nicer, but there is still a lot of syntax (calls to begin(), end(),
iteration increments) hiding what is a very simple objective: loop over
every item in the container.  In fact, it seems clearest to use the new
for-range syntax.

-- Code with for-range syntax
for (auto p : subfcn_names)
-- End Code

This is okay from a functional perspective--it compiles and runs--but is
not necessarily efficient.  The mental model to use is that the for-range
loop is equivalent to a function call where the loop is the prototype
describing the function inputs and the body of the loop is the function
body.  In C++ one can pass arguments by value, by reference, or by pointer.

The example code above passes code by value.  The original prototype that
we were replacing was std::list<std::string>::const_iterator, so passing by
value does ensure that we preserve the constantness of the original list. 
But it is expensive because the compiler has to create a temporary variable
p of type std::string and then invoke a copy constructor to load it with
data each time through the for loop.  That might be okay for containers
holding simple items like std::list<int>, but most of the time it is going
to be a problem.

My coding default, and advice to others, is to pass by reference and use
const as necessary to preserve the original intent of the code.  So, the
example now becomes

-- Code with for-range syntax
for (const auto& p : subfcn_names)
-- End Code

While changing over instances of iterators to use auto I have followed the
original code's lead in using 'const' or not.  Sometimes, it seems pretty
clear that a const_iterator would have been a better choice, but I don't
want to conflate switching to the for-range syntax and also debugging a
bunch more Octave code.

Finally, variable naming now becomes extremely important.  The initial
code, by explicitly stating the exact type of the variable p, gave a lot of
clues as to how p was to be used.  That is not the case anymore.  Of
course, this is a general problem faced in scripting languages where
variables are polymorphic.  One best practice is to name the variable after
what it does, or at a minimum what it is (type).  If I see a variable
called "filename", I can be reasonably certain that it holds a string.  Or
if it is a string, but not with some obvious purpose such as holding a
filename, then it might be named "tmpstr" which has the type name embedded
in it.

As an example, std::map<Type1, Type2>, is used pretty frequently in the
Octave code.  Here is how I changed one instance.

-- Code with variable naming
-      for (bp_table::fname_bp_map_iterator it = bp_list.begin ();
-           it != bp_list.end (); it++)

+      for (auto& fnm_bp_p: bp_list)
-- End Code

In this case, bp_table is a map with the key being the filename and the
value being a list of breakpoints.  The for-range syntax returns a
std::pair<key_type, val_type> for each iteration, so I chose to use the
suffix "_p".  The overall variable name is now "fnm_bp_p".

Converting the whole of the Octave code base over to C++11 syntax will take
a long time, but I'm trying to do just a file at a time.  Anyone can take
part in this effort.  Too see a list of for loops with iterators that might
qualify, try

cd libinterp
grep -rn 'for ' * | grep -v iterator > ../iter.list

Cheers,
Rik




reply via email to

[Prev in Thread] Current Thread [Next in Thread]