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.

Sign up for GitHub

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

Jump to bottom

Marking unused code #4661

Merged
merged 2 commits into from Mar 26, 2024
Merged

Marking unused code #4661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb3dc13
(test/gurka/test_instructions_roundabout.cc) marking unused code
cvvergara Mar 26, 2024
f03601e
(test/gurka/test_instructions_named_roundabout.cc) marking unused code
cvvergara Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion test/gurka/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -12,7 +12,6 @@ if(ENABLE_DATA_TOOLS)
# Avoid adding a filename to this list
set(TESTS_WITH_WARNINGS
test_gtfs.cc
test_instructions_roundabout.cc
test_languages.cc
test_locate.cc
test_multi_level_loki.cc
Expand Down
13 changes: 8 additions & 5 deletions test/gurka/test_instructions_named_roundabout.cc
View file
Open in desktop
Expand Up @@ -71,11 +71,14 @@ TEST_F(InstructionsNamedRoundabout, RoundaboutEnterOnly) {

// TODO: known issue - future update to end on a roundabout
// Verify the enter_roundabout instructions
// gurka::assert::raw::expect_instructions_at_maneuver_index(result, maneuver_index,
// "Enter the roundabout.",
// "Enter Dupont Circle.",
// "Enter Dupont Circle.",
// "Enter Dupont Circle.", "");
#if 0
int maneuver_index = 1;
gurka::assert::raw::expect_instructions_at_maneuver_index(result, maneuver_index,
"Enter the roundabout.",
"Enter Dupont Circle.",
"Enter Dupont Circle.",
"Enter Dupont Circle.", "");
#endif
}

// enter_roundabout_verbal
Expand Down
12 changes: 7 additions & 5 deletions test/gurka/test_instructions_roundabout.cc
View file
Open in desktop
Expand Up @@ -69,11 +69,13 @@ TEST_F(InstructionsRoundabout, RoundaboutEnterOnly) {

// TODO: known issue - future update to end on a roundabout
// Verify the enter_roundabout instructions
// gurka::assert::raw::expect_instructions_at_maneuver_index(result, maneuver_index,
// "Enter the roundabout.", "Enter
// the roundabout.", "Enter the
// roundabout.", "Enter the
// roundabout.", "");
#if 0
Copy link
Member

@nilsnolde nilsnolde Mar 26, 2024 •
edited

Choose a reason for hiding this comment

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

was that a "multiline comment" warning? I always wondered why that would be a compiler warning.. to me, it's kinda a case in point that we shouldn't use -WAll/Extra at all and instead choose the few compiler warnings we actually want to care about. there's many very good ones, but this isn't one of them IMHO.

that would also solve the annoying thing where we semi-regularly break the -WAll CI builds because some compiler update introduced some esoteric new warning which we then have to comply with.

it's just unfortunately a serious PITA to write out all interesting warnings.. on top of that clang & gcc here and there have different names for the same warning! thanks for that..

so yeah, in summary, I just wanna express how much I loathe these nonsense warnings 😄 I do understand why you do if 0 (easy to grep, won't be compiled) and it makes sense here instead of removing it seems, reading the comment. I still hope that we don't have to do those changes too much

Copy link
Member

Choose a reason for hiding this comment

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

my two cents: i hate this convention 😄

there is nothing wrong with commenting something out. if the compiler is warning about this then as nils said we have it configured wrong. this is just way way too pedantic to be reasonable. can we avoid doing more of this?

if we desperately want to use the preprocessor to disable this code we should define a macro for tests to use like NON_WORKING_TEST_CODE or something. the idea with these things are kind of like the author leaving a sketch of something that is TODOs but isnt ready yet. having ifdef 0 looks more like stuff you leave around while developing but want to actually delete before you PR the change

gurka::assert::raw::expect_instructions_at_maneuver_index(result, maneuver_index,
"Enter the roundabout.",
"Enter the roundabout.",
"Enter the roundabout.",
"Enter the roundabout.", "");
#endif
}

// enter_roundabout_verbal
Expand Down