Add missing returns in non-void functions #2951

2022-07-12T21:47:49Z

jaimeMF commented

2022-07-12 21:47:49 +00:00

(Migrated from github.com)

Distributions like openSUSE compile packages with the "-Werror=return-type".

Signed-off-by: Jaime Marquínez Ferrándiz jaime.marquinez.ferrandiz@fastmail.net

Distributions like openSUSE compile packages with the "-Werror=return-type". Signed-off-by: Jaime Marquínez Ferrándiz <jaime.marquinez.ferrandiz@fastmail.net>

biodranik (Migrated from github.com) requested changes 2022-07-13 04:52:50 +00:00

biodranik (Migrated from github.com) left a comment

What is your compiler version? Modern compilers are smart enough to handle these cases.

3party/opening_hours/rules_evaluation.cpp

biodranik (Migrated from github.com) commented

2022-07-13 04:49:36 +00:00

No, it will silence the error if a new switch case will be added.

generator/generator_tests/collector_building_parts_tests.cpp

					
				@ -30,7 +30,7 @@ public:

				  }

biodranik (Migrated from github.com) commented

2022-07-13 04:49:57 +00:00

https://en.cppreference.com/w/cpp/language/attributes/noreturn

generator/generator_tests/relation_tags_tests.cpp

					
				@ -22,7 +22,7 @@ public:

				  }

biodranik (Migrated from github.com) commented

2022-07-13 04:50:04 +00:00

https://en.cppreference.com/w/cpp/language/attributes/noreturn

geometry/clipping.cpp

biodranik (Migrated from github.com) commented

2022-07-13 04:50:33 +00:00

No, it will silence the error if a new switch case will be added.

geometry/clipping.cpp

biodranik (Migrated from github.com) commented

2022-07-13 04:50:38 +00:00

No, it will silence the error if a new switch case will be added.

routing/cross_mwm_graph.cpp

biodranik (Migrated from github.com) commented

2022-07-13 04:51:52 +00:00

No, it will silence the error if a new switch case will be added. The code should not return in this case, but CRASH immediately, so we can fix the issue before releasing it to the public.

routing/fake_vertex.hpp

biodranik (Migrated from github.com) commented

2022-07-13 04:52:15 +00:00

No, it will silence the error if a new switch case will be added.

jaimeMF commented

2022-07-13 06:16:05 +00:00

(Migrated from github.com)

What is your compiler version? Modern compilers are smart enough to handle these cases.

> g++ --version 
g++ (SUSE Linux) 12.1.1 20220629 [revision 7811663964aa7e31c3939b859bbfa2e16919639f]

I added -Werror=return-type to CMAKE_CXX_FLAGS to reproduce how the rpm would be built and it gets called with the following arguments:

[ 12%] Building CXX object 3party/opening_hours/CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o
cd /home/jaime/code/proyectos/openstreetmap/build-organicmaps-Desktop-Debug/3party/opening_hours && /usr/bin/g++ -DDEBUG -I/home/jaime/code/proyectos/openstreetmap/organicmaps -I/home/jaime/code/proyectos/openstreetmap/organicmaps/3party/boost -Werror=return-type -g -fPIC   -fno-omit-frame-pointer -fdiagnostics-color=always -ffast-math -Wall -Wextra -Wno-unused-parameter -Wno-deprecated-copy -std=c++17 -MD -MT 3party/opening_hours/CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o -MF CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o.d -o CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o -c /home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp
/home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp: In function 'osmoh::RuleState {anonymous}::ModifierToRuleState(osmoh::RuleSequence::Modifier)':
/home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp:114:1: error: control reaches end of non-void function [-Werror=return-type]
  114 | }
      | ^
cc1plus: some warnings being treated as errors

> What is your compiler version? Modern compilers are smart enough to handle these cases. ``` > g++ --version g++ (SUSE Linux) 12.1.1 20220629 [revision 7811663964aa7e31c3939b859bbfa2e16919639f] ``` I added `-Werror=return-type` to CMAKE_CXX_FLAGS to reproduce how the rpm would be built and it gets called with the following arguments: ``` [ 12%] Building CXX object 3party/opening_hours/CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o cd /home/jaime/code/proyectos/openstreetmap/build-organicmaps-Desktop-Debug/3party/opening_hours && /usr/bin/g++ -DDEBUG -I/home/jaime/code/proyectos/openstreetmap/organicmaps -I/home/jaime/code/proyectos/openstreetmap/organicmaps/3party/boost -Werror=return-type -g -fPIC -fno-omit-frame-pointer -fdiagnostics-color=always -ffast-math -Wall -Wextra -Wno-unused-parameter -Wno-deprecated-copy -std=c++17 -MD -MT 3party/opening_hours/CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o -MF CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o.d -o CMakeFiles/opening_hours.dir/rules_evaluation.cpp.o -c /home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp /home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp: In function 'osmoh::RuleState {anonymous}::ModifierToRuleState(osmoh::RuleSequence::Modifier)': /home/jaime/code/proyectos/openstreetmap/organicmaps/3party/opening_hours/rules_evaluation.cpp:114:1: error: control reaches end of non-void function [-Werror=return-type] 114 | } | ^ cc1plus: some warnings being treated as errors ```

biodranik commented

2022-07-13 06:22:33 +00:00

(Migrated from github.com)

That's not ok, the current implementation is pretty well defined according to the C++17 standard. Anyway, if you are really interested to merge it, different fixes are needed.

jaimeMF commented

2022-07-13 15:09:10 +00:00

(Migrated from github.com)

I added the noreturn attribute where you asked

That's not ok, the current implementation is pretty well defined according to the C++17 standard. Anyway, if you are really interested to merge it, different fixes are needed.

Since I have little experience with C++ and the codebase, could you point to some other place in the project where it's done?

I added the noreturn attribute where you asked > That's not ok, the current implementation is pretty well defined according to the C++17 standard. Anyway, if you are really interested to merge it, different fixes are needed. Since I have little experience with C++ and the codebase, could you point to some other place in the project where it's done?

biodranik commented

2022-07-13 20:40:56 +00:00

(Migrated from github.com)

It should be simple in most cases: do not use the default case that silences important compiler errors when a new enum member is added.

jaimeMF commented

2022-07-13 22:12:45 +00:00

(Migrated from github.com)

It should be simple in most cases: do not use the default case that silences important compiler errors when a new enum member is added.

Ok, so I should do this instead?


inline std::string DebugPrint(FakeVertex::Type type)
{
  switch (type)
  {
  case FakeVertex::Type::PureFake: return "PureFake";
  case FakeVertex::Type::PartOfReal: return "PartOfReal";
  }
  return "UnkonwFakeVertexType";
}

> It should be simple in most cases: do not use the default case that silences important compiler errors when a new enum member is added. Ok, so I should do this instead? ```c++ inline std::string DebugPrint(FakeVertex::Type type) { switch (type) { case FakeVertex::Type::PureFake: return "PureFake"; case FakeVertex::Type::PartOfReal: return "PartOfReal"; } return "UnkonwFakeVertexType"; } ```

biodranik commented

2022-07-14 05:13:23 +00:00

(Migrated from github.com)

Yes.

jaimeMF commented

2022-07-14 18:40:12 +00:00

(Migrated from github.com)

I have removed the incorrect usage of default. I can squash the commits if you want.

biodranik (Migrated from github.com) reviewed 2022-07-14 22:28:25 +00:00

routing/fake_vertex.hpp

biodranik (Migrated from github.com) commented

2022-07-14 22:28:24 +00:00

  return "UnknownFakeVertexType";

```suggestion return "UnknownFakeVertexType"; ```

biodranik (Migrated from github.com) requested changes 2022-07-14 22:29:26 +00:00

biodranik (Migrated from github.com) left a comment

Please insert this line before each unreachable return:
CHECK(false, ("Unreachable"));

Please insert this line before each unreachable return: `CHECK(false, ("Unreachable"));`

jaimeMF commented

2022-07-15 15:44:25 +00:00

(Migrated from github.com)

Please insert this line before each unreachable return: CHECK(false, ("Unreachable"));

Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable?

> Please insert this line before each unreachable return: `CHECK(false, ("Unreachable"));` Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable?

biodranik commented

2022-07-15 19:56:56 +00:00

(Migrated from github.com)

Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable?

No, third parties should:

#include <cstdlib>  // std::abort
#include <iostream>  // std::cerr

std::cerr << "Unreachable\n";
std::abort();

> Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable? No, third parties should: ``` #include <cstdlib> // std::abort #include <iostream> // std::cerr ``` ``` std::cerr << "Unreachable\n"; std::abort(); ```

jaimeMF commented

2022-07-15 20:14:34 +00:00

(Migrated from github.com)

Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable?

No, third parties should:
#include <cstdlib>  // std::abort
#include <iostream>  // std::cerr
std::cerr << "Unreachable\n";
std::abort();

Done.

> > Done. But I had to link the opening_hours to the base library in order to use it, is it acceptable? > > No, third parties should: > > ``` > #include <cstdlib> // std::abort > #include <iostream> // std::cerr > ``` > > ``` > std::cerr << "Unreachable\n"; > std::abort(); > ``` Done.

biodranik (Migrated from github.com) approved these changes 2022-07-17 00:10:23 +00:00

aaronpuchert (Migrated from github.com) reviewed 2022-07-30 14:55:28 +00:00

3party/opening_hours/rules_evaluation.cpp

aaronpuchert (Migrated from github.com) commented

2022-07-30 14:55:28 +00:00

This line should probably be removed. It has no effect (std::abort has [[noreturn]]) and will likely trigger -Wunreachable-code-return.

This line should probably be removed. It has no effect (`std::abort` [has](https://en.cppreference.com/w/cpp/utility/program/abort) `[[noreturn]]`) and will likely trigger [-Wunreachable-code-return](https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-return).

This repo is archived. You cannot comment on pull requests.

No reviewers

biodranik