Reduce the usage of auto keyword in SMTK code base

Simulation Modeling Toolkit (SMTK) Software Process
1

Hi SMTK developers,

auto is a cool feature introduced by C++11 and it can be handy in many situations. However we are some kind of abusing it in SMTK code base as it breaks the readability and causes extra work.

Meaningful variable name can be a solution here but it’s not enough… Ex. auto faces = Foo(). Clearly it’s a bunch of faces but what’s the underlying container? A set? A vector? I have to check the signature of Foo then could I figure out the proper APIs for faces.

I’m proposing we use auto with more caution and specify specific type whenever feasible.

2

I couldn’t agree more. Sadly, this is a “code style” debate (akin to requiring better variable names), and is therefore hard to enforce. I have been fixing unnecessary instances of auto as I come across them, but it would be nice for there to be a more official policy on its use.

3

There are many places where auto is useful and not ambiguous. I think it is important to leave those alone. For example:

auto item = std::static_pointer_cast<smtk::attribute::DoubleItemDefinition>(this->ItemDef);

The type is clear from the right-hand side of the assignment and the typename is loooong. Repeating it is bad.

4

Yes, in this case auto is great. This is not the issue.

The trouble is when auto is used to refer to things like ParaView class instances (like auto thingy = rep->getProxy()->GetClientSideObject()) where there are several layers of misdirection to get to the API of the class that has been accessed.

Because this issue falls under style, it is difficult to put a hard rule on when to use auto. I think we should follow Justice Stewart’s lead and instead focus on readability; the next person who has to decipher your code may be you!

5

I agree this is crystal clear. And it should be encouraged. Also for VTK style iterators, auto can also be useful(Moreover once we bump ParaView to past 5.6, we can use the cool vtkRange added by Allie).
However following code is bad…


  auto ductThickness = core.ductThickness();

  const auto& segments = duct.segments();

  // Follow logic in cmbNucRender::createGeo function. L168
  // Create a name-auxgeom map so that we can assign the right rep
  AuxiliaryGeometries childrenAux = ductAux.auxiliaryGeometries();
  std::map<std::string, AuxiliaryGeometry*> nameToChildAux;
  for (auto aux : childrenAux)
  {
    nameToChildAux[aux.name()] = &aux;
  }

As the author of rgg session i know these auto types without a single thought. But it’s bad code as it adds extra burden to future developers.

+1 for I know it when I see it (here I refers to a developer who first time sees the code!)

6

Who would do something awful like that? :crazy_face:

+1