Skip to content

Commit

Permalink
Code Quality: Address compiler warnings
Browse files Browse the repository at this point in the history
AOS-31877
AOS-73372

- Fixing narrowing issues
- Removing useless copies
- Removing unused lines
- unused-lambda-capture
- Removes unused variables
- Fixes some casts
- Explicitly deleting unused endpoint_impl copy and move constructors
- Removing redundant std::bind
- Improving const correctness
- Moving thread init to constructor body
- Moved check_routing_credentials_ inside vsomeip security section where
  it's used
- Using =default destructor instead of empty destructor

Thread init:
Moving the initialization of these threads into the constructor body to
ensure that they do not start with an incomplete "this".  As they
capture this, it is possible that if the new thread begins before the
object is fully constructed, the new thread might operate on
uninitialized members of "this".
  • Loading branch information
kheaactua committed Nov 3, 2023
1 parent 27242c2 commit 22fdbb1
Show file tree
Hide file tree
Showing 24 changed files with 70 additions and 71 deletions.
12 changes: 6 additions & 6 deletions examples/hello_world/hello_world_service.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
#if defined ANDROID || defined __ANDROID__
#include "android/log.h"
#define LOG_TAG "hello_world_service"
#define LOG_INF(...) fprintf(stdout, __VA_ARGS__), fprintf(stdout, "\n"), (void)__android_log_print(ANDROID_LOG_INFO, LOG_TAG, ##__VA_ARGS__)
#define LOG_ERR(...) fprintf(stderr, __VA_ARGS__), fprintf(stderr, "\n"), (void)__android_log_print(ANDROID_LOG_ERROR, LOG_TAG, ##__VA_ARGS__)
#define LOG_INF(...) std::fprintf(stdout, __VA_ARGS__), std::fprintf(stdout, "\n"), (void)__android_log_print(ANDROID_LOG_INFO, LOG_TAG, ##__VA_ARGS__)
#define LOG_ERR(...) std::fprintf(stderr, __VA_ARGS__), std::fprintf(stderr, "\n"), (void)__android_log_print(ANDROID_LOG_ERROR, LOG_TAG, ##__VA_ARGS__)
#else
#include <cstdio>
#define LOG_INF(...) fprintf(stdout, __VA_ARGS__), fprintf(stdout, "\n")
#define LOG_ERR(...) fprintf(stderr, __VA_ARGS__), fprintf(stderr, "\n")
#define LOG_INF(...) std::fprintf(stdout, __VA_ARGS__), std::fprintf(stdout, "\n")
#define LOG_ERR(...) std::fprintf(stderr, __VA_ARGS__), std::fprintf(stderr, "\n")
#endif

static vsomeip::service_t service_id = 0x1111;
Expand All @@ -32,9 +32,9 @@ class hello_world_service {
hello_world_service() :
rtm_(vsomeip::runtime::get()),
app_(rtm_->create_application()),
stop_(false),
stop_thread_(std::bind(&hello_world_service::stop, this))
stop_(false)
{
stop_thread_ = std::thread{&hello_world_service::stop, this};
}

~hello_world_service()
Expand Down
6 changes: 3 additions & 3 deletions implementation/configuration/src/configuration_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ bool configuration_impl::load(const std::string &_name) {

// Tell, if reading of configuration file(s) failed.
// (This may file if the logger configuration is incomplete/missing).
for (auto f : its_failed)
for (auto const& f : its_failed)
VSOMEIP_WARNING << "Reading of configuration file \""
<< f << "\" failed. Configuration may be incomplete.";

Expand All @@ -331,7 +331,7 @@ bool configuration_impl::load(const std::string &_name) {

std::chrono::steady_clock::time_point end = std::chrono::steady_clock::now();

for (auto i : its_input) {
for (auto const& i : its_input) {
if (utility::is_file(i))
VSOMEIP_INFO << "Using configuration file: \"" << i << "\".";

Expand Down Expand Up @@ -550,7 +550,7 @@ bool configuration_impl::load_data(const std::vector<configuration_element> &_el

if (is_logging_loaded_) {
logger::logger_impl::init(shared_from_this());
for (auto w : its_warnings)
for (auto const& w : its_warnings)
VSOMEIP_WARNING << w;
}
}
Expand Down
3 changes: 3 additions & 0 deletions implementation/endpoints/include/endpoint_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class endpoint_impl: public virtual endpoint {
std::uint32_t _max_message_size,
configuration::endpoint_queue_limit_t _queue_limit,
const std::shared_ptr<configuration>& _configuration);
endpoint_impl(endpoint_impl<Protocol> const&) = delete;
endpoint_impl(endpoint_impl<Protocol> const&&) = delete;

virtual ~endpoint_impl();

void enable_magic_cookies();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ struct local_server_endpoint_impl_receive_op {

socket_type_t &socket_;
receive_handler_t handler_;
byte_t *buffer_;
byte_t *buffer_ = nullptr;
size_t length_;
uid_t uid_;
gid_t gid_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ struct udp_server_endpoint_impl_receive_op {
socket_type_t &socket_;
endpoint_type_t &sender_;
receive_handler_t handler_;
byte_t *buffer_;
byte_t *buffer_ = nullptr;
size_t length_;
uint8_t multicast_id_;
bool is_v4_;
Expand Down
2 changes: 1 addition & 1 deletion implementation/endpoints/src/endpoint_manager_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ std::shared_ptr<endpoint> endpoint_manager_base::create_local(client_t _client)
return create_local_unlocked(_client);
}

void endpoint_manager_base::remove_local(client_t _client) {
void endpoint_manager_base::remove_local(client_t const _client) {
std::shared_ptr<endpoint> its_endpoint(find_local(_client));
if (its_endpoint) {
its_endpoint->register_error_handler(nullptr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,9 @@ local_uds_client_endpoint_impl::local_uds_client_endpoint_impl(
is_supporting_magic_cookies_ = false;
}

local_uds_client_endpoint_impl::~local_uds_client_endpoint_impl() {

}
local_uds_client_endpoint_impl::~local_uds_client_endpoint_impl() = default;

bool local_uds_client_endpoint_impl::is_local() const {

return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,13 @@ local_uds_server_endpoint_impl::local_uds_server_endpoint_impl(
#endif
}

local_uds_server_endpoint_impl::~local_uds_server_endpoint_impl() {

}
local_uds_server_endpoint_impl::~local_uds_server_endpoint_impl() = default;

bool local_uds_server_endpoint_impl::is_local() const {

return true;
}

void local_uds_server_endpoint_impl::start() {

std::lock_guard<std::mutex> its_lock(acceptor_mutex_);
if (acceptor_.is_open()) {
connection::ptr new_connection = connection::create(
Expand Down
2 changes: 1 addition & 1 deletion implementation/endpoints/src/tcp_client_endpoint_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ void tcp_client_endpoint_impl::connect() {
std::string its_device(configuration_->get_device());
if (its_device != "") {
if (setsockopt(socket_->native_handle(),
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), (socklen_t)its_device.size()) == -1) {
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), socklen_t(its_device.size())) == -1) {
VSOMEIP_WARNING << "TCP Client: Could not bind to device \"" << its_device << "\"";
}
}
Expand Down
14 changes: 7 additions & 7 deletions implementation/endpoints/src/tcp_server_endpoint_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ tcp_server_endpoint_impl::tcp_server_endpoint_impl(
std::string its_device(configuration_->get_device());
if (its_device != "") {
if (setsockopt(acceptor_.native_handle(),
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), (socklen_t)its_device.size()) == -1) {
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), static_cast<socklen_t>(its_device.size())) == -1) {
VSOMEIP_WARNING << "TCP Server: Could not bind to device \"" << its_device << "\"";
}
}
Expand Down Expand Up @@ -295,8 +295,8 @@ void tcp_server_endpoint_impl::accept_cbk(const connection::ptr& _connection,
auto its_ep = std::dynamic_pointer_cast<tcp_server_endpoint_impl>(
shared_from_this());
its_timer->async_wait([its_timer, its_ep]
(const boost::system::error_code& _error) {
if (!_error) {
(const boost::system::error_code& _error_inner) {
if (!_error_inner) {
its_ep->start();
}
});
Expand Down Expand Up @@ -853,12 +853,12 @@ void tcp_server_endpoint_impl::connection::handle_recv_buffer_exception(
<< std::setfill('0') << std::hex;

for (std::size_t i = 0; i < recv_buffer_size_ && i < 16; i++) {
its_message << std::setw(2) << (int) (recv_buffer_[i]) << " ";
its_message << std::setw(2) << static_cast<int>(recv_buffer_[i]) << " ";
}

its_message << " Last 16 Bytes captured: ";
for (int i = 15; recv_buffer_size_ > 15 && i >= 0; i--) {
its_message << std::setw(2) << (int) (recv_buffer_[static_cast<size_t>(i)]) << " ";
its_message << std::setw(2) << static_cast<int>(recv_buffer_[static_cast<size_t>(i)]) << " ";
}
VSOMEIP_ERROR << its_message.str();
recv_buffer_.clear();
Expand Down Expand Up @@ -954,7 +954,7 @@ void tcp_server_endpoint_impl::print_status() {
std::lock_guard<std::mutex> its_lock(mutex_);
connections_t its_connections;
{
std::lock_guard<std::mutex> its_lock(connections_mutex_);
std::lock_guard<std::mutex> its_lock_inner(connections_mutex_);
its_connections = connections_;
}

Expand Down Expand Up @@ -1027,7 +1027,7 @@ void tcp_server_endpoint_impl::connection::wait_until_sent(const boost::system::
}
}
{
std::lock_guard<std::mutex> its_lock(its_server->connections_mutex_);
std::lock_guard<std::mutex> its_lock_inner(its_server->connections_mutex_);
stop();
}
its_server->remove_connection(this);
Expand Down
2 changes: 1 addition & 1 deletion implementation/endpoints/src/udp_client_endpoint_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ void udp_client_endpoint_impl::connect() {
<< get_address_port_remote();
}
socket_->set_option(boost::asio::socket_base::receive_buffer_size(
udp_receive_buffer_size_), its_error);
static_cast<int>(udp_receive_buffer_size_)), its_error);
if (its_error) {
VSOMEIP_WARNING << "udp_client_endpoint_impl::connect: couldn't set "
<< "SO_RCVBUF: " << its_error.message()
Expand Down
4 changes: 2 additions & 2 deletions implementation/endpoints/src/udp_server_endpoint_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ udp_server_endpoint_impl::udp_server_endpoint_impl(
std::string its_device(configuration_->get_device());
if (its_device != "") {
if (setsockopt(unicast_socket_.native_handle(),
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), (socklen_t)its_device.size()) == -1) {
SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), socklen_t(its_device.size())) == -1) {
VSOMEIP_WARNING << "UDP Server: Could not bind to device \"" << its_device << "\"";
}
}
Expand Down Expand Up @@ -108,7 +108,7 @@ udp_server_endpoint_impl::udp_server_endpoint_impl(
const int its_udp_recv_buffer_size =
configuration_->get_udp_receive_buffer_size();
unicast_socket_.set_option(boost::asio::socket_base::receive_buffer_size(
its_udp_recv_buffer_size), ec);
static_cast<int>(its_udp_recv_buffer_size)), ec);

if (ec) {
VSOMEIP_WARNING << "udp_server_endpoint_impl: couldn't set "
Expand Down
2 changes: 0 additions & 2 deletions implementation/message/include/message_base_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
#ifndef VSOMEIP_V3_MESSAGE_BASE_IMPL_HPP
#define VSOMEIP_V3_MESSAGE_BASE_IMPL_HPP

#include <boost/thread.hpp>

#include <vsomeip/export.hpp>
#include <vsomeip/message.hpp>

Expand Down
4 changes: 2 additions & 2 deletions implementation/message/src/deserializer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ bool deserializer::deserialize(std::string &_target, std::size_t _length) {
if (_length > remaining_ || _length > _target.capacity()) {
return false;
}
_target.assign(position_, position_ + long(_length));
position_ += long(_length);
_target.assign(position_, position_ + static_cast<std::vector<byte_t>::difference_type>(_length));
position_ += static_cast<std::vector<byte_t>::difference_type>(_length);
remaining_ -= _length;

return true;
Expand Down
2 changes: 1 addition & 1 deletion implementation/plugin/src/plugin_manager_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ bool plugin_manager_impl::unload_plugin(plugin_type_e _type) {
}
} else {
VSOMEIP_ERROR << "plugin_manager_impl::unload_plugin didn't find plugin"
<< " type:" << (int)_type;
<< " type:" << static_cast<int>(_type);
return false;
}
return plugins_.erase(_type);
Expand Down
10 changes: 5 additions & 5 deletions implementation/routing/src/routing_manager_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1075,8 +1075,8 @@ void routing_manager_base::remove_local(client_t _client,
std::lock_guard<std::mutex> its_lock(local_services_mutex_);
// Finally remove all services that are implemented by the client.
std::set<std::pair<service_t, instance_t>> its_services;
for (auto& s : local_services_) {
for (auto& i : s.second) {
for (auto const& s : local_services_) {
for (auto const& i : s.second) {
if (std::get<2>(i.second) == _client) {
its_services.insert({ s.first, i.first });
host_->on_availability(s.first, i.first, availability_state_e::AS_UNAVAILABLE,
Expand All @@ -1093,9 +1093,9 @@ void routing_manager_base::remove_local(client_t _client,

// remove disconnected client from offer service history
std::set<std::tuple<service_t, instance_t, client_t>> its_clients;
for (auto& s : local_services_history_) {
for (auto& i : s.second) {
for (auto& c : i.second) {
for (auto const& s : local_services_history_) {
for (auto const& i : s.second) {
for (auto const& c : i.second) {
if (c == _client) {
its_clients.insert(std::make_tuple(s.first, i.first, c));
}
Expand Down
4 changes: 2 additions & 2 deletions implementation/runtime/include/application_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ class application_impl: public application,
std::shared_ptr<sync_handler> get_next_handler();
void reschedule_availability_handler(const std::shared_ptr<sync_handler> &_handler);
bool has_active_dispatcher();
bool is_active_dispatcher(const std::thread::id &_id);
bool is_active_dispatcher(const std::thread::id &_id) const;
void remove_elapsed_dispatchers();

void shutdown();
Expand Down Expand Up @@ -426,7 +426,7 @@ class application_impl: public application,
// Dispatcher threads that are running
std::set<std::thread::id> running_dispatchers_;
// Mutex to protect access to dispatchers_ & elapsed_dispatchers_
std::mutex dispatcher_mutex_;
mutable std::mutex dispatcher_mutex_;

// Condition to wakeup the dispatcher thread
mutable std::condition_variable dispatcher_condition_;
Expand Down
7 changes: 4 additions & 3 deletions implementation/runtime/src/application_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,8 @@ void application_impl::start() {
std::lock_guard<std::mutex> its_lock(dispatcher_mutex_);
is_dispatching_ = true;
auto its_main_dispatcher = std::make_shared<std::thread>(
std::bind(&application_impl::main_dispatch, shared_from_this()));
&application_impl::main_dispatch, shared_from_this()
);
dispatchers_[its_main_dispatcher->get_id()] = its_main_dispatcher;
}

Expand Down Expand Up @@ -1774,7 +1775,7 @@ void application_impl::main_dispatch() {
}
} else {
std::shared_ptr<sync_handler> its_handler;
while (is_dispatching_ && is_active_dispatcher(its_id)
while (is_dispatching_ && is_active_dispatcher(its_id)
&& (its_handler = get_next_handler())) {
its_lock.unlock();
invoke_handler(its_handler);
Expand Down Expand Up @@ -2030,7 +2031,7 @@ bool application_impl::has_active_dispatcher() {
return false;
}

bool application_impl::is_active_dispatcher(const std::thread::id &_id) {
bool application_impl::is_active_dispatcher(const std::thread::id &_id) const {
while (is_dispatching_) {
if (dispatcher_mutex_.try_lock()) {
for (const auto &d : dispatchers_) {
Expand Down
3 changes: 2 additions & 1 deletion implementation/security/include/policy_manager_impl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,11 @@ class VSOMEIP_IMPORT_EXPORT policy_manager_impl
mutable boost::shared_mutex policy_extension_paths_mutex_;
//map[hostname, pair[path, map[complete path with UID/GID, control loading]]
std::map<std::string, std::pair<std::string, std::map<std::string, bool>>> policy_extension_paths_;

bool check_routing_credentials_;
#endif // !VSOMEIP_DISABLE_SECURITY

bool is_configured_;
bool check_routing_credentials_;

mutable std::mutex routing_credentials_mutex_;
std::pair<uint32_t, uint32_t> routing_credentials_;
Expand Down
4 changes: 2 additions & 2 deletions implementation/security/src/policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ policy::deserialize_ids(const byte_t * &_data, uint32_t &_size,
if (its_result == false)
return false;

for (const auto i : its_instances)
for (const auto& i : its_instances)
its_ids += std::make_pair(i, its_methods);

its_array_length -= (its_current_size - _size);
Expand Down Expand Up @@ -379,7 +379,7 @@ policy::serialize_interval_set(
uint32_t its_interval_set_size(0);
serialize_u32(its_interval_set_size, _data);

for (const auto i : _intervals)
for (const auto& i : _intervals)
serialize_interval(i, _data);

its_interval_set_size = static_cast<uint32_t>(_data.size()
Expand Down
4 changes: 2 additions & 2 deletions implementation/security/src/policy_manager_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ policy_manager_impl::policy_manager_impl()
allow_remote_clients_(true),
check_whitelist_(false),
policy_base_path_(""),
check_routing_credentials_(false),
#endif // !VSOMEIP_DISABLE_SECURITY
is_configured_(false),
check_routing_credentials_(false)
is_configured_(false)
{
}

Expand Down
1 change: 1 addition & 0 deletions implementation/security/src/security.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "../../plugin/include/plugin_manager.hpp"

#include <array>
#include <iomanip>
#include <tuple>

#ifndef _WIN32
Expand Down
Loading

0 comments on commit 22fdbb1

Please sign in to comment.