ament / ament_lint

Apache License 2.0
37 stars 107 forks source link

Ament uncrustify does not break line in multi-line constructor call #231

Open rotu opened 4 years ago

rotu commented 4 years ago

It seems ament_uncrustify misses a required newline in the below code. A line break is required after filter(, but for some reason, ament_uncrustify fails to enforce it.

  // Filter for only 'use_sim_time' being added or changed.
  rclcpp::ParameterEventsFilter filter(event, {"use_sim_time"},
    {rclcpp::ParameterEventsFilter::EventType::NEW,
      rclcpp::ParameterEventsFilter::EventType::CHANGED});

https://github.com/ros2/rclcpp/blob/ee6ab95cfc8ecab353d4cf1efdb81de237424f11/rclcpp/src/rclcpp/time_source.cpp#L247-L250

dirk-thomas commented 4 years ago

Do you think this is related to the uncrustify configuration file provided by the package or a bug in uncrustify itself? If it is the latter please create a ticket in the upstream uncrustify repository instead.

rotu commented 4 years ago

Investigating... It may even be a bug of the pinned version of uncrustify in uncrustify_vendor. Nevertheless, even if it's a problem in the upstream code, it's a functional shortcoming that ought to be documented against ament_lint somehow, since the intent is to faithfully enforce the coding standards.

rotu commented 4 years ago

Alright. It seems that uncrustify (current HEAD version) does not treat object direct initialization (of the form var Class(args ...)) as function calls for purposes of nl_func_call_start_multi_line and I can't find a configuration option

I can see 3 possible resolutions. I'm leaning toward 2 or 3:

  1. I'm interpreting the developer guide wrong and the formatting guidelines for function calls differ from direct initialization.
  2. This is a bug in uncrustify and direct initialization should be considered function calls.
  3. This is a missing feature in uncrustify, and we can't expect the uncrustify linter to enforce this part of the developer guide.