guybrush77 / rapidobj

A fast, header-only, C++17 library for parsing Wavefront .obj files.
MIT License
159 stars 14 forks source link

{cmake} Declare include directories as SYSTEM #21

Closed asmaloney closed 11 months ago

asmaloney commented 11 months ago

This allows consumers of the header to ignore all the warnings so -Werror may be used.

See: https://cmake.org/cmake/help/latest/command/target_include_directories.html

(If you would rather I go through and fix the warnings I can work on that. Looks like it's mostly shadowed vars and unused vars in if statements.)

guybrush77 commented 11 months ago

Thanks for the pull request. rapidobj includes two third-party libraries, fast_float and earcut. Do you know if the warnings occur in these libraries, in rapidobj code or both?

asmaloney commented 11 months ago

It looks like they are in the rapidobj code.

I'm not getting the warnings locally (Apple Clang), but they show up on CI (13.1.0 on Linux). Using -Wall -Wextra -Wpedantic -Wshadow -Wunused along with several others...

Here are a few:

extern/rapidobj/include/rapidobj/rapidobj.hpp: In lambda function:
extern/rapidobj/include/rapidobj/rapidobj.hpp:6145:49: error: declaration of 'task' shadows a previous local [-Werror=shadow]
 6145 |         if (auto rc = std::visit([](const auto& task) { return task.Execute(); }, task); rc != rapidobj_errc::Success) {
      |                                     ~~~~~~~~~~~~^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:6143:21: note: shadowed declaration is here
 6143 |         const auto& task = tasks[fetched_index];
      |                     ^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp: In function 'rapidobj::Result rapidobj::detail::ParseFile(const std::filesystem::__cxx11::path&, const rapidobj::MaterialLibrary&)':
extern/rapidobj/include/rapidobj/rapidobj.hpp:7133:22: error: unused variable 'string' [-Werror=unused-variable]
 7133 |     } else if (auto* string = std::get_if<std::string_view>(material_library_value)) {
      |                      ^~~~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:7126:22: error: unused variable 'none' [-Werror=unused-variable]
 7126 |     } else if (auto* none = std::get_if<std::monostate>(material_library_value)) {
      |                      ^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:7124:15: error: unused variable 'null' [-Werror=unused-variable]
 7124 |     if (auto* null = std::get_if<std::nullptr_t>(material_library_value)) {
      |               ^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp: In function 'rapidobj::Result rapidobj::detail::ParseStream(std::istream&, const rapidobj::MaterialLibrary&)':
extern/rapidobj/include/rapidobj/rapidobj.hpp:7214:22: error: unused variable 'string' [-Werror=unused-variable]
 7214 |     } else if (auto* string = std::get_if<std::string_view>(material_library_value)) {
      |                      ^~~~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:7201:22: error: unused variable 'none' [-Werror=unused-variable]
 7201 |     } else if (auto* none = std::get_if<std::monostate>(material_library_value)) {
      |                      ^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:7199:15: error: unused variable 'null' [-Werror=unused-variable]
 7199 |     if (auto* null = std::get_if<std::nullptr_t>(material_library_value)) {
      |               ^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp: In constructor 'rapidobj::detail::TriangulateTask::TriangulateTask(const rapidobj::Mesh*, rapidobj::Mesh*, size_t, size_t, size_t, size_t, size_t, size_t)':
extern/rapidobj/include/rapidobj/rapidobj.hpp:7268:21: error: declaration of 'size' shadows a member of 'rapidobj::detail::TriangulateTask' [-Werror=shadow]
 7268 |         size_t      size) noexcept
      |         ~~~~~~~~~~~~^~~~
extern/rapidobj/include/rapidobj/rapidobj.hpp:7279:17: note: shadowed declaration is here
 7279 |     size_t      size{};
guybrush77 commented 11 months ago

If you don't mind fixing warnings in rapidobj code, that would be great and much appreciated!

We can still merge this PR after the warnings have been fixed.