facontidavide / ros_type_introspection

Deserialize ROS messages that are unknown at compilation time
MIT License
61 stars 30 forks source link

gcc 7.4.0 gives [-Wmaybe-uninitialized] on code using variant.hpp's convert function #38

Closed aPonza closed 5 years ago

aPonza commented 5 years ago

Compiling

void printMessage(const RosIntrospection::FlatMessage& flat_container)
{
  for (auto& it : flat_container.value)
    if (it.second.convert<std::uint8_t>()) // error value, of type franka_msgs::Errors::_tau_j_range_violation_type
      ROS_ERROR(" - %s", it.first.toStdString().c_str()); // error name
}

with gcc 7.4.0 -Wall -O3 returns:

<...>/source.cpp: In function ‘void ns::printMessage(const RosIntrospection::FlatMessage&)’:
<...>/source.cpp:892:5: warning: ‘target’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (it.second.convert<std::uint8_t>())

Looking into it, I found -Wmaybe-uninitialized's cause and tried to see if I'm in that case, but I can't see any issues.

Now I'm not discounting being wrong, but if I add a default case to the convert function in variant.hpp, the warning disappears:

  case OTHER: throw TypeException("Variant::convert -> cannot convert type" + std::to_string(_type)); break;

  default: throw TypeException("Variant::convert -> cannot convert type" + std::to_string(_type)); break;

  }
  return  target;

Also, I hadn't realized until this last copy-paste, but target is exactly the return variable of the convert function.

I'd actually suggest changing the OTHER type (actually most of the times I see enums, the last one is the total number of enumerated cases, i.e. an ALL_TYPES in this case. It'd probably be useless though) with the default case.

EDIT: no, not changing the OTHER type, but introducing the default where you anyways throw

facontidavide commented 5 years ago

Your suggestion makes sense, and I believe that I undertand why GCC complains. I will apply the change. Thanks for reporting