iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for `cxx::function_ref` #1456

mossmaurice · 2022-07-06T13:37:56Z

Signed-off-by: Simon Hoinkis [email protected]

Pre-Review Checklist for the PR Author

Notes for Reviewer

Address AUTOSAR warnings with Axivion
Fix clang-tidy warnings in cxx::function_ref
Findings regardings Expects not considered will be addressed as part of ErrorHandling concept with user defined actions #1032 by @MatthiasKillat

Todo

Some suppressions not considered by Axivion -> Fixed
~~M7-1-2: Shall constness of 'target' be changed?~~
Discuss A2-7-3 and A14-5-2

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
- Each unit test case has a unique UUID
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Address Autosar Violations using Axivion #1394 (partly)
Closes Enable clang-tidy checks and fix warnings in code #1196 (partly)

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

codecov · 2022-07-06T14:42:13Z

Codecov Report

Merging #1456 (16fd6c2) into master (ebc67df) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1456      +/-   ##
==========================================
+ Coverage   78.75%   78.76%   +0.01%     
==========================================
  Files         379      379              
  Lines       14477    14477              
  Branches     2012     2012              
==========================================
+ Hits        11401    11403       +2     
+ Misses       2436     2435       -1     
+ Partials      640      639       -1

Flag	Coverage Δ
unittests	`78.42% <100.00%> (+0.01%)`	⬆️
unittests_timing	`14.87% <75.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nclude/iceoryx_hoofs/internal/cxx/function_ref.inl	`93.33% <100.00%> (ø)`
iceoryx_posh/source/mepoo/memory_manager.cpp	`97.08% <0.00%> (+1.94%)`	⬆️

elfenpiff · 2022-07-13T09:24:20Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

@@ -15,15 +15,15 @@
 //
 // SPDX-License-Identifier: Apache-2.0

+// AXIVION DISABLE STYLE AutosarC++19_03-A16.2.3 : <type_traits> is included


Actually I have no idea what this is suppressing and why a <type_traits> makes it safe again. Could you elaborate this a little bit more detailed. With other warnings it is most of the time more clear since they are directly above the line which has the "warning" and one sees the context but in this case it could be easily also a cut&paste failure.

I think it is about type_traits from STL being used but only included indirectly. I think the proper fix is to include it directly, as is customary in C++.

@elfenpiff @MatthiasKillat For <type_traits> we have our trampoline type_traits.hpp which adds some type traits missing in C++14. I re-phrased the justification.

elfenpiff · 2022-07-13T09:32:32Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

  public:
    ~function_ref() noexcept = default;

    function_ref(const function_ref&) noexcept = default;

-    function_ref& operator=(const function_ref&) noexcept = default;
+    function_ref& operator=(const function_ref&) & noexcept = default;


Why did you add a reference here? Was it a warning? Should we do this for all copy assignments?

So this rule forbids code like:

std::move(a) = b;

Which is valid and well defined but on the first glimpse weird. But take a look at this here

template<typename T> void initializeSomeArgs(T&& args) { std::forward<T>(args) = someT; }

Here we take args as universal reference (so sometimes also as rvalue reference) and would like to assign a new value to it.
Why should we forbid that? What kind of error can possible originate from that?

@elfenpiff In the example you provided it can have outside effect, depending on the type it is instantiated with so anything can go wrong but this is no reason not to allow since this holds for any references or pointers.

While it will not be useful often (or at all) for function_ref, I do not see it should be required to restrict the assignment operator to lvalues.

@mossmaurice Edit: I thought about this. For function_ref there is probably no good use case to assign to rvalues so it is fine to prohibit this explicitly. Same for all other places where I mentioned it.

It should not be done for all classes without considering their purpose, but it is rarely useful I think.

@elfenpiff That's due to AUTOSAR rule A12-8-7. Indeed

std::move(a) = b;

is weird and does not make sense. You can find more examples in the AUTOSAR document.

For your 2nd example, I suppose if it is needed in some cases, we should explicitly define foo& operator=(const foo&) && and if desired call operator=() & from there explicitly.

If both implementations (&& and &) exist and are identical it does not make sense to distinguish. Only if one should not exist (as here) or if they both exist but are different.

It is fine if we only have one like here.

elfenpiff · 2022-07-13T09:34:06Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

 namespace iox
 {
 namespace cxx
 {
 template <class ReturnType, class... ArgTypes>
 template <typename CallableType, typename>
+// AXIVION Next Line AutosarC++19_03-A12.1.2 : Members are initialized in the same manner, NSDMI with nullptr is


In what manner are the members initialized? What does NSDMI mean?

https://www.cppstories.com/2015/02/non-static-data-members-initialization/

But I think this Axivion suppression is probably not the right way, @saif-at-github may know more (should it suppress the next Construct instead?). I do see any members in the next line and am not sure what this rule is about or why there is an issue.

Ideally there is nothing to suppress here, I cannot the any problem.

@elfenpiff https://www.giybf.com 🙃

@MatthiasKillat Correct, I switchted it to Next Construct.

The rule just says either init via the : m_foo(foo) or inline via Type m_foo{defaultValue}, but don't mix things as you might forget it in another c'tor. However, doing it explicitly in both ways, is the best IMHO.

elfenpiff · 2022-07-13T09:35:04Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

    : m_pointerToCallable(const_cast<void*>(reinterpret_cast<const void*>(std::addressof(callable))))
+    // AXIVION Next Line AutosarC++19_03-A15.4.4 : Lambda not 'noexcept' as callable might throw


With this argument the call operator must be noexcept.

15-4-4 states that "A declaration of non-throwing function shall contain noexcept specification.". I have no problem with that. However, any function that might throw cannot have this specificier, which means any user-defined function cannot. Hence I do not see why Axivion should complain unless it is better analyzing whether a function can throw or not than the compiler.

And even modern compilers are in general not able to do this in general, non-trivial cases (like this one). Not sure the rule is useful to enforce with this tool for this reason. If the tool produces too many false positives/negatives it is useless for this rule (while the rule itself makes sense).

@elfenpiff I don't get this, the operator()() is already noexcept.

@MatthiasKillat Correct, we need to monitor that.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

MatthiasKillat · 2022-07-14T12:42:02Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

@@ -15,15 +15,15 @@
 //
 // SPDX-License-Identifier: Apache-2.0

+// AXIVION DISABLE STYLE AutosarC++19_03-A16.2.3 : <type_traits> is included


I think it is about type_traits from STL being used but only included indirectly. I think the proper fix is to include it directly, as is customary in C++.

MatthiasKillat · 2022-07-14T12:43:37Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

 #ifndef IOX_HOOFS_CXX_FUNCTION_REF_HPP
 #define IOX_HOOFS_CXX_FUNCTION_REF_HPP

+// AXIVION Next Line AutosarC++19_03-A16.2.2 : Needed for Expects and Ensures macros


What is the problem here? Does it not recognize that something from this header is used? (maybe because it is a macro?) ...

@MatthiasKillat AFAIU Axivion does not detect that the macro from requires.hpp is used.

I dislike that tool failure results in these suppression comments.

-1 for the tool I guess

MatthiasKillat · 2022-07-14T12:51:51Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

  public:
    ~function_ref() noexcept = default;

    function_ref(const function_ref&) noexcept = default;

-    function_ref& operator=(const function_ref&) noexcept = default;
+    function_ref& operator=(const function_ref&) & noexcept = default;


@elfenpiff In the example you provided it can have outside effect, depending on the type it is instantiated with so anything can go wrong but this is no reason not to allow since this holds for any references or pointers.

While it will not be useful often (or at all) for function_ref, I do not see it should be required to restrict the assignment operator to lvalues.

@mossmaurice Edit: I thought about this. For function_ref there is probably no good use case to assign to rvalues so it is fine to prohibit this explicitly. Same for all other places where I mentioned it.

It should not be done for all classes without considering their purpose, but it is rarely useful I think.

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

MatthiasKillat · 2022-07-14T15:04:57Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

-        auto f = reinterpret_cast<ReturnType (*)(ArgTypes...)>(target);
+    m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
+        using PointerType = ReturnType (*)(ArgTypes...);
+        // AXIVION Next Construct AutosarC++19_03-A5.2.4 : Type-safety ensured by casting from type


Type safety is ensured by the fact we only pass functions with this type as target, i.e. they are convertible.

This is entirely under the control of the class.

@mossmaurice I still find the justification lacking here, hence my comment about the reason.

Without extra knowledge of how it works, I could not infer it from the justification.

More precisely
the class design ensures a cast to the actual type of target

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice · 2022-07-15T10:26:53Z

@elfenpiff @MatthiasKillat @elBoberido

The combination of clang-tidy and Axivion suppression has been confirmed to work in a call with Axivion yesterday:

// AXIVION Next Construct Ruleset-A1.2.3 : Because foo and bar
// NOLINTNEXTLINE (foobar)

MatthiasKillat

Looks good. I think some justifications wrt. type erasure are not quite clear and need some detail/rewording.

MatthiasKillat · 2022-07-15T11:00:24Z

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

  public:
    ~function_ref() noexcept = default;

    function_ref(const function_ref&) noexcept = default;

-    function_ref& operator=(const function_ref&) noexcept = default;
+    function_ref& operator=(const function_ref&) & noexcept = default;


If both implementations (&& and &) exist and are identical it does not make sense to distinguish. Only if one should not exist (as here) or if they both exist but are different.

It is fine if we only have one like here.

iceoryx_hoofs/include/iceoryx_hoofs/cxx/function_ref.hpp

MatthiasKillat · 2022-07-15T11:16:59Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

    , m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
+        // AXIVION Next Construct AutosarC++19_03-A5.2.4, CertC++-EXP36 : Type-safety ensured by casting from type


@mossmaurice I still think the justification is not accurate/understandable. type erasure should be mentioned.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

MatthiasKillat · 2022-07-15T11:20:55Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

-        auto f = reinterpret_cast<ReturnType (*)(ArgTypes...)>(target);
+    m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
+        using PointerType = ReturnType (*)(ArgTypes...);
+        // AXIVION Next Construct AutosarC++19_03-A5.2.4 : Type-safety ensured by casting from type


@mossmaurice I still find the justification lacking here, hence my comment about the reason.

Without extra knowledge of how it works, I could not infer it from the justification.

More precisely
the class design ensures a cast to the actual type of target

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice added the AUTOSAR Warning of an Adpative Autosar C++14 rule label Jul 6, 2022

mossmaurice requested review from dkroenke and MatthiasKillat July 6, 2022 13:37

mossmaurice self-assigned this Jul 6, 2022

mossmaurice changed the title ~~iox-#1394 Address Axivion findings for cxx::function_ref~~ iox-#1394 Address Axivion findings for cxx::function_ref Jul 6, 2022

mossmaurice commented Jul 6, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/function_ref.inl Outdated Show resolved Hide resolved

mossmaurice added the CERT Warning of a SEI CERT C++ 2016 rule label Jul 6, 2022

mossmaurice force-pushed the iox-#1394-fix-axivion-warning-in-function-ref branch from 5fcd3c6 to bbcf4c2 Compare July 6, 2022 13:47

mossmaurice mentioned this pull request Jul 7, 2022

iox-#1196 Fix clang-tidy warnings in cxx::function_ref #1468

Closed

21 tasks

mossmaurice marked this pull request as ready for review July 11, 2022 09:12

mossmaurice added the clang-tidy Warning of a clang-tidy rule label Jul 11, 2022

mossmaurice changed the title ~~iox-#1394 Address Axivion findings for cxx::function_ref~~ iox-#1394 iox-#1196 Address Axivion findings for cxx::function_ref Jul 11, 2022

mossmaurice force-pushed the iox-#1394-fix-axivion-warning-in-function-ref branch from 757e67e to 1840354 Compare July 11, 2022 09:19

mossmaurice changed the title ~~iox-#1394 iox-#1196 Address Axivion findings for cxx::function_ref~~ iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for cxx::function_ref Jul 11, 2022

dkroenke previously approved these changes Jul 12, 2022

View reviewed changes

elfenpiff reviewed Jul 13, 2022

View reviewed changes

elfenpiff removed the request for review from MatthiasKillat July 13, 2022 09:39

MatthiasKillat reviewed Jul 14, 2022

View reviewed changes

mossmaurice added 3 commits July 15, 2022 10:41

iox-eclipse-iceoryx#1394 Address Axivion findings for cxx::function_ref

e5bffb0

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#1196 Fix clang-tidy warnings in cxx::function_ref

f49d5fc

Signed-off-by: Simon Hoinkis <[email protected]>

iox-eclipse-iceoryx#1394 Fix clang-tidy warnings in function_ref tests

c9c54dd

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice dismissed dkroenke’s stale review via c9c54dd July 15, 2022 09:23

mossmaurice force-pushed the iox-#1394-fix-axivion-warning-in-function-ref branch from 1840354 to c9c54dd Compare July 15, 2022 09:23

mossmaurice requested review from dkroenke, elfenpiff and MatthiasKillat July 15, 2022 10:22

MatthiasKillat reviewed Jul 15, 2022

View reviewed changes

iox-eclipse-iceoryx#1394 Add missing declaration of swap to .hpp

16fd6c2

Signed-off-by: Simon Hoinkis <[email protected]>

mossmaurice force-pushed the iox-#1394-fix-axivion-warning-in-function-ref branch from ffd5a26 to 16fd6c2 Compare July 15, 2022 12:01

mossmaurice requested a review from MatthiasKillat July 15, 2022 12:01

MatthiasKillat approved these changes Jul 15, 2022

View reviewed changes

dkroenke approved these changes Jul 15, 2022

View reviewed changes

mossmaurice merged commit 9537beb into eclipse-iceoryx:master Jul 15, 2022

mossmaurice deleted the iox-#1394-fix-axivion-warning-in-function-ref branch July 15, 2022 15:06

mossmaurice mentioned this pull request Jul 19, 2022

iox-#1196 Fix left-over clang-tidy warnings in cxx::vector and remove space in suppressions #1525

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for `cxx::function_ref` #1456

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for `cxx::function_ref` #1456

mossmaurice commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 6, 2022 •

edited

Loading

elfenpiff Jul 13, 2022

MatthiasKillat Jul 14, 2022

mossmaurice Jul 15, 2022

elfenpiff Jul 13, 2022

MatthiasKillat Jul 14, 2022 •

edited

Loading

mossmaurice Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022 •

edited

Loading

elfenpiff Jul 13, 2022

MatthiasKillat Jul 14, 2022

mossmaurice Jul 15, 2022

elfenpiff Jul 13, 2022

MatthiasKillat Jul 14, 2022

mossmaurice Jul 15, 2022

MatthiasKillat Jul 14, 2022

MatthiasKillat Jul 14, 2022

mossmaurice Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 14, 2022 •

edited

Loading

MatthiasKillat Jul 14, 2022

MatthiasKillat Jul 15, 2022

mossmaurice commented Jul 15, 2022

MatthiasKillat left a comment

MatthiasKillat Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022

MatthiasKillat Jul 15, 2022

		: m_pointerToCallable(const_cast<void>(reinterpret_cast<const void>(std::addressof(callable))))
		// AXIVION Next Line AutosarC++19_03-A15.4.4 : Lambda not 'noexcept' as callable might throw

		, m_functionPointer([](void* target, ArgTypes... args) -> ReturnType {
		// AXIVION Next Construct AutosarC++19_03-A5.2.4, CertC++-EXP36 : Type-safety ensured by casting from type

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for cxx::function_ref #1456

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for cxx::function_ref #1456

Conversation

mossmaurice commented Jul 6, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Todo

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

codecov bot commented Jul 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

mossmaurice Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossmaurice commented Jul 15, 2022

MatthiasKillat left a comment

Choose a reason for hiding this comment

MatthiasKillat Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for `cxx::function_ref` #1456

iox-#1394 iox-#1196 Address Axivion and clang-tidy findings for `cxx::function_ref` #1456

mossmaurice commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 6, 2022 •

edited

Loading

MatthiasKillat Jul 14, 2022 •

edited

Loading

mossmaurice Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022 •

edited

Loading

mossmaurice Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022 •

edited

Loading

MatthiasKillat Jul 14, 2022 •

edited

Loading

MatthiasKillat Jul 15, 2022 •

edited

Loading