Skip to content

Commit

Permalink
PR fixes #verification
Browse files Browse the repository at this point in the history
1. Less `auto` according AUTOSAR A7-1-5
2. Less `struct` according AUTOSAR A11-0-2
3. C.35: A base class destructor should be either public and virtual, or protected and non-virtual.
4. `IMedia::setFilters` now fallible.
  • Loading branch information
serges147 committed Apr 22, 2024
1 parent 3e0b4bb commit 80d0ea7
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 30 deletions.
8 changes: 4 additions & 4 deletions include/libcyphal/transport/can/media.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class IMedia
IMedia(const IMedia&) = delete;
IMedia& operator=(IMedia&&) = delete;
IMedia& operator=(const IMedia&) = delete;
virtual ~IMedia() = default;

/// @brief Get the maximum transmission unit (MTU) of the CAN bus.
///
Expand All @@ -55,9 +56,9 @@ class IMedia
/// While reconfiguration is in progress, incoming frames may be lost and/or unwanted frames may be received.
/// The lifetime of the filter array may end upon return (no references retained).
///
/// @return Returns `true` on success, `false` in case of a low-level error (e.g., IO error).
/// @return Returns `nullopt` on success; otherwise some `MediaError` in case of a low-level error.
///
CETL_NODISCARD virtual bool setFilters(const Filters filters) noexcept = 0;
CETL_NODISCARD virtual cetl::optional<MediaError> setFilters(const Filters filters) noexcept = 0;

/// @brief Schedules the frame for transmission asynchronously and return immediately.
///
Expand All @@ -76,8 +77,7 @@ class IMedia
const cetl::span<cetl::byte> payload_buffer) noexcept = 0;

protected:
IMedia() = default;
virtual ~IMedia() = default;
IMedia() = default;

}; // IMedia

Expand Down
31 changes: 12 additions & 19 deletions include/libcyphal/transport/can/transport.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ class TransportImpl final : public ICanTransport
}
CETL_NODISCARD ProtocolParams getProtocolParams() const noexcept override
{
const auto min_mtu = reduceMediaInto(std::numeric_limits<std::size_t>::max(),
[](auto&& mtu, IMedia& media) { mtu = std::min(mtu, media.getMtu()); });
const auto min_mtu =
reduceMediaInto(std::numeric_limits<std::size_t>::max(),
[](std::size_t&& mtu, IMedia& media) { mtu = std::min(mtu, media.getMtu()); });
return ProtocolParams{1 << CANARD_TRANSFER_ID_BIT_LENGTH, min_mtu, CANARD_NODE_ID_MAX + 1};
}

Expand Down Expand Up @@ -99,7 +100,7 @@ class TransportImpl final : public ICanTransport
// TODO: Remove this workaround when the issue is resolved.
// see https://github.com/OpenCyphal/libcanard/issues/216
//
struct CanardMemory
struct CanardMemory final
{
alignas(std::max_align_t) std::size_t size;
};
Expand All @@ -116,8 +117,8 @@ class TransportImpl final : public ICanTransport
{
auto& self = getSelfFrom(ins);

const auto memory_size = sizeof(CanardMemory) + amount;
auto canard_memory = static_cast<CanardMemory*>(self.memory_.allocate(memory_size));
const std::size_t memory_size = sizeof(CanardMemory) + amount;
auto canard_memory = static_cast<CanardMemory*>(self.memory_.allocate(memory_size));
if (canard_memory == nullptr)
{
return nullptr;
Expand All @@ -144,20 +145,10 @@ class TransportImpl final : public ICanTransport
self.memory_.deallocate(canard_memory, canard_memory->size);
}

template <typename Action>
void forEachMedia(Action action)
{
for (const auto media : media_array_)
{
CETL_DEBUG_ASSERT(media != nullptr, "Expected media interface.");
action(std::ref(*media));
}
}

template <typename T, typename Reducer>
T reduceMediaInto(T&& init, Reducer reducer) const
{
for (const auto media : media_array_)
for (IMedia* const media : media_array_)
{
CETL_DEBUG_ASSERT(media != nullptr, "Expected media interface.");
reducer(std::forward<T>(init), std::ref(*media));
Expand Down Expand Up @@ -188,8 +179,8 @@ CETL_NODISCARD inline Expected<UniquePtr<ICanTransport>, FactoryError> makeTrans
// - At least one media interface must be provided.
// - If a local node ID is provided, it must be within the valid range.
//
const auto media_count =
static_cast<std::size_t>(std::count_if(media.cbegin(), media.cend(), [](auto m) { return m != nullptr; }));
const auto media_count = static_cast<std::size_t>(
std::count_if(media.cbegin(), media.cend(), [](IMedia* const m) { return m != nullptr; }));
if (media_count == 0)
{
return ArgumentError{};
Expand All @@ -201,7 +192,9 @@ CETL_NODISCARD inline Expected<UniquePtr<ICanTransport>, FactoryError> makeTrans

libcyphal::detail::VarArray<IMedia*> media_array{MaxMediaInterfaces, &memory};
media_array.reserve(media_count);
std::copy_if(media.cbegin(), media.cend(), std::back_inserter(media_array), [](auto m) { return m != nullptr; });
std::copy_if(media.cbegin(), media.cend(), std::back_inserter(media_array), [](IMedia* const m) {
return m != nullptr;
});
CETL_DEBUG_ASSERT(!media_array.empty() && (media_array.size() == media_count), "");

const auto canard_node_id = static_cast<CanardNodeID>(local_node_id.value_or(CANARD_NODE_ID_UNSET));
Expand Down
2 changes: 1 addition & 1 deletion include/libcyphal/transport/errors.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ using FactoryError = cetl::variant<ArgumentError, MemoryError, NotImplementedErr

/// @brief Defines any possible error at Cyphal media layer.
///
using MediaError = cetl::variant<ArgumentError, PlatformError>;
using MediaError = cetl::variant<ArgumentError, PlatformError, CapacityError>;

} // namespace transport
} // namespace libcyphal
Expand Down
4 changes: 2 additions & 2 deletions include/libcyphal/transport/multiplexer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ class IMultiplexer
IMultiplexer(const IMultiplexer&) = delete;
IMultiplexer& operator=(IMultiplexer&&) = delete;
IMultiplexer& operator=(const IMultiplexer&) = delete;
virtual ~IMultiplexer() = default;

// TODO: Add methods here

protected:
IMultiplexer() = default;
virtual ~IMultiplexer() = default;
IMultiplexer() = default;

}; // IMultiplexer

Expand Down
4 changes: 2 additions & 2 deletions include/libcyphal/transport/udp/media.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ class IMedia
IMedia(const IMedia&) = delete;
IMedia& operator=(IMedia&&) = delete;
IMedia& operator=(const IMedia&) = delete;
virtual ~IMedia() = default;

// TODO: Add methods here

protected:
IMedia() = default;
virtual ~IMedia() = default;
IMedia() = default;

}; // IMedia

Expand Down
2 changes: 1 addition & 1 deletion test/unittest/transport/can/media_mock.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class MediaMock : public IMedia
{
public:
MOCK_METHOD(std::size_t, getMtu, (), (const, noexcept, override));
MOCK_METHOD(bool, setFilters, (const Filters filters), (noexcept, override));
MOCK_METHOD(cetl::optional<MediaError>, setFilters, (const Filters filters), (noexcept, override));
MOCK_METHOD((Expected<bool, MediaError>),
push,
(const TimePoint deadline, const CanId can_id, const cetl::span<const cetl::byte> payload),
Expand Down
2 changes: 1 addition & 1 deletion test/unittest/transport/test_dynamic_buffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class InterfaceMock : public DynamicBuffer::Interface
MOCK_METHOD(std::size_t, size, (), (const, noexcept, override));
MOCK_METHOD(std::size_t, copy, (const std::size_t, void* const, const std::size_t), (const, override));
};
class InterfaceWrapper : public rtti_helper<TestTypeIdType, DynamicBuffer::Interface>
class InterfaceWrapper final : public rtti_helper<TestTypeIdType, DynamicBuffer::Interface>
{
public:
explicit InterfaceWrapper(InterfaceMock* mock)
Expand Down

0 comments on commit 80d0ea7

Please sign in to comment.