Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed compiler warnings #2313

Open
wants to merge 12 commits into
base: ros2
Choose a base branch
from
Open

Fixed compiler warnings #2313

wants to merge 12 commits into from

Conversation

rociopv06
Copy link
Contributor

Fixed the warnings all around the code base. Hopefully this will facilitate debugging and should not affect any previous performance.

Copy link
Contributor

@sid-parikh sid-parikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my bad. i started this review weeks ago and never finished it.

robocup-software Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please uncommit this. you have cloned our repo inside itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try rm -rf robocup-software (make sure you are inside the outer robocup-software first!)

@@ -163,7 +163,7 @@ void Camera::update_balls_mhkf(RJ::Time calc_time, const std::vector<CameraBall>
const CameraBall& camera_ball = ball_list.at(i);
bool was_used = used_camera_ball.at(i);

if (!was_used && kalman_ball_list_.size() < PARAM_max_num_kalman_balls) {
if (!was_used && (int64_t) kalman_ball_list_.size() < PARAM_max_num_kalman_balls) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!was_used && (int64_t) kalman_ball_list_.size() < PARAM_max_num_kalman_balls) {
if (!was_used && static_cast<int64_t>(kalman_ball_list_.size()) < PARAM_max_num_kalman_balls) {

or you could maybe do

Suggested change
if (!was_used && (int64_t) kalman_ball_list_.size() < PARAM_max_num_kalman_balls) {
if (!was_used && static_cast<decltype(PARAM_max_num_kalman_balls)>(kalman_ball_list_.size()) < PARAM_max_num_kalman_balls) {

i'm not sure the best practice, anything is fine really, but no C-style casts please.

double max_acc = 1;
double start_speed = 0.901393;
double final_speed = 0;
[[maybe_unused]] double pos_out = 1.11101;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if these are unused can we just delete them?

@@ -112,7 +112,7 @@ class DebugDrawer {
/// Debug layers in order by ID
QStringList debug_layers_;

Context* context_;
[[maybe_unused]] Context* context_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems really sus...

@@ -30,7 +30,7 @@ Trajectory PathTargetPathPlanner::plan(const PlanRequest& request) {
}

LinearMotionInstant target_instant = command.target;
Point goal_point = target_instant.position;
[[maybe_unused]] Point goal_point = target_instant.position;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, local vars that are unused should just be deleted.

@@ -197,7 +197,7 @@ void RobotFactoryPosition::start_kicker_picker() {
bool RobotFactoryPosition::have_all_kicker_responses() {
int num_alive = std::count(alive_robots_.begin(), alive_robots_.end(), true);

return kicker_distances_.size() == num_alive - 1; // Don't expect the goalie to respond
return (int)kicker_distances_.size() == num_alive - 1; // Don't expect the goalie to respond
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static cast

@@ -30,8 +30,24 @@ class Zoner : public Position {
~Zoner() override = default;
Zoner(const Position& other);
Zoner(Zoner&& other) = default;
Zoner& operator=(const Zoner& other) = default;
Zoner& operator=(Zoner&& other) = default;
// Zoner& operator=(const Zoner& other) = default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're actually right here, this is pretty frustrating; it's why const members are very discouraged. however, i'm scared to write these explicitly because we'd have to maintain them if we ever add members. i would say it's better to either explicitly = delete these operators (we rarely would have a use for them anyway) or simply remove the const qualifiaction from the member (i don't even see it, but i have a hunch it could be robot_id in the super class)

@@ -341,7 +341,7 @@ void MainWindow::updateViews() {
QString("Log: %1 kiB").arg(QString::number((context_->logs.size_bytes + 512) / 1024)));
}

auto value = _ui.logHistoryLocation->value();
[[maybe_unused]] auto value = _ui.logHistoryLocation->value();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again [[maybe_unused]] local variable declarations seems like an anti-pattern, probably just remove them?

@@ -95,9 +95,9 @@ TEST(KalmanRobot, predict) {
kb.predict(t);

rj_geometry::Point rp2 = kb.get_pos();
rj_geometry::Point rv2 = kb.get_vel();
[[maybe_unused]] rj_geometry::Point rv2 = kb.get_vel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -4,7 +4,7 @@
#include "ui_RefereeTab.h"

class RefereeTab : public QWidget {
Q_OBJECT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. (but wow i hate QT)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants