mavros/0e2ea0c: check updated description

[gavanderhoorn]

The previous version stated a std::string was checked with .size() > 0 for 'empty-ness', with the fix changing that to empty() (implying it's better in some way).

I've corrected the description, as it was in fact a std::map, not a std::string:

https://github.com/robust-rosin/robust/blob/a8b60a374dfefd9c57a58e25ebe82ac8be0566d1/mavros/0e2ea0c/0e2ea0c.bug#L1-L14

Disregarding the std::string vs std::map part: looking at the implementation of std::map::empty(), it basically checks _M_impl._M_node_count == 0 (where _M_impl is the backing store for the map, in this case an stl_tree). That doesn't look too different from size() > 0 (size() returns the node count, which is not calculated ad-hoc for size(), but is a member variable kept up-to-date across insertions, removals, etc).

I wasn't able to find any cppcheck reference which states that size() is bad and empty() is good (or better).

@wasowski @ChrisTimperley @git-afsantos @ipa-hsd: do any of you recognise these phrases and could perhaps point to a source for this claim?

[gavanderhoorn]

Perhaps it's the possibility the backing-store could have implemented size() in an expensive way? Not sure how that differs from the 'danger' of empty() being expensive though.

[ChrisTimperley]

For quick reference, here's the link to the fix commit: https://github.com/mavlink/mavros/commit/0e2ea0c43b373feb490b45710fb63f4e6cde78eb

The fix commit mentions cppcheck but doesn't say anything about efficiency. I suspect that this is a general code style / code smell issue and that we (probably me 🙂) have incorrectly made performance assumptions (that would be true in other languages/contexts but not in this case).

[gavanderhoorn]

So then the only 'optimisation' would be the change to pass-by-ref.

Do we feel that's enough to warrant a SOFTWARE:PERFORMANCE code?

[git-afsantos]

Probably not. Besides, it seems that the other change was included in the fix, despite not being related to the issue. I would say that this has nothing to do with performance, but rather with readability.

[ChrisTimperley]

^ Agreed with @git-afsantos

[gavanderhoorn]

So any suggestions for a new failure-code? SYSTEM:NONE?

[git-afsantos]

If the issue is just size() > 0 vs empty(), then yes. There is nothing one could observe at the system level that would manifest from this tiny change.

[ChrisTimperley]

This seems like a code smell (and not a bug) to me

[gavanderhoorn]

Fixed: bfb1e21952c99235c0435c817a4f8f823bc28ed5