From ba94da173015c20f35d6d59b4658912af7ee0b12 Mon Sep 17 00:00:00 2001 From: Steve Hollasch Date: Sat, 13 Feb 2021 19:50:06 -0800 Subject: [PATCH 1/3] Lint pass: ctor initializer order + virtual dtors This commit enables new warnings for the MSVC compiler: - C4265: Class has virtual functions, but its non-trivial destructor is not virtual. - C5204: Class has virtual functions, but its trivial destructor is not virtual. - C5038: Data member will be initialized after [other] data member The first two warnings catch cases where destructors don't cascade when someone forgets to specify that a derived class's destructor is virtual. The third warning flags the situation where a class's data members are specified in one order, and the initializers are listed in a different order. Member variables are always initialized in class declaration order, so programmers who expect initialization in initializer list order may be surprised, particularly when some members depend on other members being initialized first. I've cleaned the code and books from the above warnings. --- CMakeLists.txt | 16 ++++++++++-- books/RayTracingInOneWeekend.html | 4 +++ books/RayTracingTheNextWeek.html | 17 ++++++++----- books/RayTracingTheRestOfYourLife.html | 34 +++++++++----------------- src/InOneWeekend/hittable.h | 2 ++ src/InOneWeekend/material.h | 2 ++ src/TheNextWeek/aarect.h | 6 ++--- src/TheNextWeek/constant_medium.h | 2 +- src/TheNextWeek/hittable.h | 2 ++ src/TheNextWeek/material.h | 2 ++ src/TheRestOfYourLife/aarect.h | 6 ++--- src/TheRestOfYourLife/hittable.h | 2 ++ src/TheRestOfYourLife/material.h | 2 ++ src/TheRestOfYourLife/pdf.h | 10 ++++---- src/common/texture.h | 4 ++- 15 files changed, 68 insertions(+), 43 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 969de878..7df7818a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -17,6 +17,8 @@ set ( CMAKE_CXX_EXTENSIONS OFF ) set ( COMMON_ALL src/common/rtweekend.h src/common/camera.h + src/common/color.h + src/common/interval.h src/common/ray.h src/common/vec3.h ) @@ -71,6 +73,18 @@ set ( SOURCE_REST_OF_YOUR_LIFE src/TheRestOfYourLife/main.cc ) +include_directories(src/common) + +# Specific Compiler Flags + +message (STATUS "Compiler ID: " ${CMAKE_CXX_COMPILER_ID}) + +if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") + add_compile_options("/we 4265") # Class has virtual functions, but its non-trivial destructor is not virtual + add_compile_options("/w3 5038") # Data member will be initialized after [other] data member + add_compile_options("/we 5204") # Class has virtual functions, but its trivial destructor is not virtual +endif() + # Executables add_executable(inOneWeekend ${SOURCE_ONE_WEEKEND}) add_executable(theNextWeek ${SOURCE_NEXT_WEEK}) @@ -82,5 +96,3 @@ add_executable(pi src/TheRestOfYourLife/pi.cc ${CO add_executable(estimate_halfway src/TheRestOfYourLife/estimate_halfway.cc ${COMMON_ALL}) add_executable(sphere_importance src/TheRestOfYourLife/sphere_importance.cc ${COMMON_ALL}) add_executable(sphere_plot src/TheRestOfYourLife/sphere_plot.cc ${COMMON_ALL}) - -include_directories(src/common) diff --git a/books/RayTracingInOneWeekend.html b/books/RayTracingInOneWeekend.html index d4917ca3..007a4f69 100644 --- a/books/RayTracingInOneWeekend.html +++ b/books/RayTracingInOneWeekend.html @@ -867,6 +867,8 @@ class hittable { public: + virtual ~hittable() = default; + virtual bool hit(const ray& r, double ray_tmin, double ray_tmax, hit_record& rec) const = 0; }; @@ -2033,6 +2035,8 @@ class material { public: + virtual ~material() = default; + virtual bool scatter( const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered) const = 0; }; diff --git a/books/RayTracingTheNextWeek.html b/books/RayTracingTheNextWeek.html index 1022b1c8..bb89864e 100644 --- a/books/RayTracingTheNextWeek.html +++ b/books/RayTracingTheNextWeek.html @@ -1099,6 +1099,8 @@ class texture { public: + virtual ~texture() = default; + virtual color value(double u, double v, const point3& p) const = 0; }; @@ -1194,8 +1196,8 @@ public: double inv_scale; - shared_ptr odd; shared_ptr even; + shared_ptr odd; }; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Listing [checker-texture]: [texture.h] Checkered texture] @@ -2293,12 +2295,15 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ class material { public: + ... + + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight virtual color emitted(double u, double v, const point3& p) const { return color(0,0,0); } - ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ + virtual bool scatter( const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered ) const = 0; @@ -2466,8 +2471,8 @@ aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double x0, x1, y0, y1, k; + shared_ptr mat; aabb bbox; }; @@ -2590,8 +2595,8 @@ aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double x0, x1, z0, z1, k; + shared_ptr mat; aabb bbox; }; @@ -2625,8 +2630,8 @@ aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double y0, y1, z0, z1, k; + shared_ptr mat; aabb bbox; }; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -3120,8 +3125,8 @@ public: shared_ptr boundary; - shared_ptr phase_function; double neg_inv_density; + shared_ptr phase_function; }; #endif diff --git a/books/RayTracingTheRestOfYourLife.html b/books/RayTracingTheRestOfYourLife.html index 453ba0b3..fcf024d4 100644 --- a/books/RayTracingTheRestOfYourLife.html +++ b/books/RayTracingTheRestOfYourLife.html @@ -1504,9 +1504,7 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ class material { public: - virtual color emitted(double u, double v, const point3& p) const { - return color(0,0,0); - } + ... ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight @@ -2305,6 +2303,9 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ class material { public: + ... + + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight virtual color emitted( const ray& r_in, const hit_record& rec, double u, double v, const point3& p @@ -2312,7 +2313,6 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ return color(0,0,0); } - ... }; @@ -2523,21 +2523,21 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ class hittable_pdf : public pdf { public: - hittable_pdf(const hittable_list& _hittable_ptr, const point3& _origin) - : hittable_ptr(_hittable_ptr), origin(_origin) + hittable_pdf(const hittable_list& _objects, const point3& _origin) + : objects(_objects), origin(_origin) {} double value(const vec3& direction) const override { - return hittable_ptr->pdf_value(origin, direction); + return objects.pdf_value(origin, direction); } vec3 generate() const override { - return hittable_ptr->random(origin); + return objects.random(origin); } public: + const hittable_list& objects; point3 origin; - shared_ptr hittable_ptr; }; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Listing [class-hittable-pdf]: [pdf.h] The hittable_pdf class] @@ -2555,9 +2555,7 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ class hittable { public: - virtual bool hit(const ray& r, interval ray_t, hit_record& rec) const = 0; - - virtual aabb bounding_box() const = 0; + ... ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight @@ -2890,11 +2888,7 @@ class material { public: - virtual color emitted( - const ray& r_in, const hit_record& rec, double u, double v, const point3& p - ) const { - return color(0,0,0); - } + ... virtual bool scatter( ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C++ highlight @@ -2903,11 +2897,7 @@ ) const { return false; } - - virtual double scattering_pdf(const ray& r_in, const hit_record& rec, const ray& scattered) - const { - return 0; - } + ... }; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [Listing [material-refactor]: [material.h] Refactoring the material class] diff --git a/src/InOneWeekend/hittable.h b/src/InOneWeekend/hittable.h index 6c9bb916..4ef172cc 100644 --- a/src/InOneWeekend/hittable.h +++ b/src/InOneWeekend/hittable.h @@ -33,6 +33,8 @@ class hit_record { class hittable { public: + virtual ~hittable() = default; + virtual bool hit(const ray& r, interval ray_t, hit_record& rec) const = 0; }; diff --git a/src/InOneWeekend/material.h b/src/InOneWeekend/material.h index f3c5ff1e..01d58bae 100644 --- a/src/InOneWeekend/material.h +++ b/src/InOneWeekend/material.h @@ -19,6 +19,8 @@ class hit_record; class material { public: + virtual ~material() = default; + virtual bool scatter( const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered ) const = 0; diff --git a/src/TheNextWeek/aarect.h b/src/TheNextWeek/aarect.h index 4eb84c3a..f992e455 100644 --- a/src/TheNextWeek/aarect.h +++ b/src/TheNextWeek/aarect.h @@ -49,8 +49,8 @@ class xy_rect : public hittable { aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double x0, x1, y0, y1, k; + shared_ptr mat; aabb bbox; }; @@ -88,8 +88,8 @@ class xz_rect : public hittable { aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double x0, x1, z0, z1, k; + shared_ptr mat; aabb bbox; }; @@ -127,8 +127,8 @@ class yz_rect : public hittable { aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double y0, y1, z0, z1, k; + shared_ptr mat; aabb bbox; }; diff --git a/src/TheNextWeek/constant_medium.h b/src/TheNextWeek/constant_medium.h index ed239f2a..57991546 100644 --- a/src/TheNextWeek/constant_medium.h +++ b/src/TheNextWeek/constant_medium.h @@ -79,8 +79,8 @@ class constant_medium : public hittable { public: shared_ptr boundary; - shared_ptr phase_function; double neg_inv_density; + shared_ptr phase_function; }; diff --git a/src/TheNextWeek/hittable.h b/src/TheNextWeek/hittable.h index 9dd3ee8d..37829e28 100644 --- a/src/TheNextWeek/hittable.h +++ b/src/TheNextWeek/hittable.h @@ -38,6 +38,8 @@ class hit_record { class hittable { public: + virtual ~hittable() = default; + virtual bool hit(const ray& r, interval ray_t, hit_record& rec) const = 0; virtual aabb bounding_box() const = 0; diff --git a/src/TheNextWeek/material.h b/src/TheNextWeek/material.h index 47087b68..e013d5b1 100644 --- a/src/TheNextWeek/material.h +++ b/src/TheNextWeek/material.h @@ -19,6 +19,8 @@ class material { public: + virtual ~material() = default; + virtual color emitted(double u, double v, const point3& p) const { return color(0,0,0); } diff --git a/src/TheRestOfYourLife/aarect.h b/src/TheRestOfYourLife/aarect.h index 3162ed3b..895f5880 100644 --- a/src/TheRestOfYourLife/aarect.h +++ b/src/TheRestOfYourLife/aarect.h @@ -49,8 +49,8 @@ class xy_rect : public hittable { aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double x0, x1, y0, y1, k; + shared_ptr mat; aabb bbox; }; @@ -105,8 +105,8 @@ class xz_rect : public hittable { } public: - shared_ptr mat; double x0, x1, z0, z1, k; + shared_ptr mat; aabb bbox; }; @@ -143,8 +143,8 @@ class yz_rect : public hittable { aabb bounding_box() const override { return bbox; } public: - shared_ptr mat; double y0, y1, z0, z1, k; + shared_ptr mat; aabb bbox; }; diff --git a/src/TheRestOfYourLife/hittable.h b/src/TheRestOfYourLife/hittable.h index 8eb58f8e..4ffe55ec 100644 --- a/src/TheRestOfYourLife/hittable.h +++ b/src/TheRestOfYourLife/hittable.h @@ -38,6 +38,8 @@ class hit_record { class hittable { public: + virtual ~hittable() = default; + virtual bool hit(const ray& r, interval ray_t, hit_record& rec) const = 0; virtual aabb bounding_box() const = 0; diff --git a/src/TheRestOfYourLife/material.h b/src/TheRestOfYourLife/material.h index b143b3c5..665b8195 100644 --- a/src/TheRestOfYourLife/material.h +++ b/src/TheRestOfYourLife/material.h @@ -28,6 +28,8 @@ class scatter_record { class material { public: + virtual ~material() = default; + virtual color emitted( const ray& r_in, const hit_record& rec, double u, double v, const point3& p ) const { diff --git a/src/TheRestOfYourLife/pdf.h b/src/TheRestOfYourLife/pdf.h index c006cc82..31f8291e 100644 --- a/src/TheRestOfYourLife/pdf.h +++ b/src/TheRestOfYourLife/pdf.h @@ -45,21 +45,21 @@ class cosine_pdf : public pdf { class hittable_pdf : public pdf { public: - hittable_pdf(const hittable_list& _hittable_ptr, const point3& _origin) - : hittable_ptr(_hittable_ptr), origin(_origin) + hittable_pdf(const hittable_list& _objects, const point3& _origin) + : objects(_objects), origin(_origin) {} double value(const vec3& direction) const override { - return hittable_ptr.pdf_value(origin, direction); + return objects.pdf_value(origin, direction); } vec3 generate() const override { - return hittable_ptr.random(origin); + return objects.random(origin); } public: + const hittable_list& objects; point3 origin; - const hittable_list& hittable_ptr; }; diff --git a/src/common/texture.h b/src/common/texture.h index 9bed30a3..ee3bc984 100644 --- a/src/common/texture.h +++ b/src/common/texture.h @@ -19,6 +19,8 @@ class texture { public: + virtual ~texture() = default; + virtual color value(double u, double v, const point3& p) const = 0; }; @@ -65,8 +67,8 @@ class checker_texture : public texture { public: double inv_scale; - shared_ptr odd; shared_ptr even; + shared_ptr odd; }; From 2c2952036ae715eb3ed1492c3be8f24f5bf3bce4 Mon Sep 17 00:00:00 2001 From: Trevor David Black Date: Mon, 22 Feb 2021 00:37:15 -0700 Subject: [PATCH 2/3] Added warnings for clang and gnu to CMake --- CMakeLists.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7df7818a..5c64a8a4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -83,6 +83,16 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") add_compile_options("/we 4265") # Class has virtual functions, but its non-trivial destructor is not virtual add_compile_options("/w3 5038") # Data member will be initialized after [other] data member add_compile_options("/we 5204") # Class has virtual functions, but its trivial destructor is not virtual +elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + add_compile_options(-Wnon-virtual-dtor) # Class has virtual functions, but its destructor is not virtual + add_compile_options(-Wreorder) # Data member will be initialized after [other] data member + add_compile_options(-Wmaybe-uninitialized) # Variable improperly initialized + add_compile_options(-Wunused-variable) # Variable is defined but unused +elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wnon-virtual-dtor) # Class has virtual functions, but its destructor is not virtual + add_compile_options(-Wreorder) # Data member will be initialized after [other] data member + add_compile_options(-Wsometimes-uninitialized) # Variable improperly initialized + add_compile_options(-Wunused-variable) # Variable is defined but unused endif() # Executables From 944fd9be326d55f9d9cd3128d1d91a1ec4a68ef9 Mon Sep 17 00:00:00 2001 From: Trevor David Black Date: Mon, 22 Feb 2021 00:48:27 -0700 Subject: [PATCH 3/3] Cleaned up warnings found in clang and gnu --- CHANGELOG.md | 1 + books/RayTracingTheRestOfYourLife.html | 3 +-- src/TheRestOfYourLife/estimate_halfway.cc | 2 +- src/TheRestOfYourLife/integrate_x_sq.cc | 2 -- src/TheRestOfYourLife/pi.cc | 1 - 5 files changed, 3 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34b07267..add361d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Change Log -- Ray Tracing in One Weekend - Change: `hittable` member variable `ptr` renamed to `object` - Change: general rename of `mat_ptr` to `mat` (material) - Change: hittable::bounding_box() signature has changed to always return a value (#859) + - Fix: Enabled compiler warnings for MSVC, Clang, GNU. Cleaned up warnings as fit (#865) ### In One Weekend - Added: More commentary about the choice between `double` and `float` (#752) diff --git a/books/RayTracingTheRestOfYourLife.html b/books/RayTracingTheRestOfYourLife.html index fcf024d4..4956e7bf 100644 --- a/books/RayTracingTheRestOfYourLife.html +++ b/books/RayTracingTheRestOfYourLife.html @@ -212,7 +212,6 @@ } } - auto N = static_cast(sqrt_N) * sqrt_N; std::cout << std::fixed << std::setprecision(12); std::cout << "Regular Estimate of Pi = " @@ -886,7 +885,7 @@ // Find out the sample at which we have half of our area double half_sum = sum / 2.0; - double halfway_point; + double halfway_point = 0.0; double accum = 0.0; for (int i = 0; i < N; i++){ accum += samples[i].p_x; diff --git a/src/TheRestOfYourLife/estimate_halfway.cc b/src/TheRestOfYourLife/estimate_halfway.cc index 23f02622..9e5f0332 100644 --- a/src/TheRestOfYourLife/estimate_halfway.cc +++ b/src/TheRestOfYourLife/estimate_halfway.cc @@ -48,7 +48,7 @@ int main() { // Find out the sample at which we have half of our area double half_sum = sum / 2.0; - double halfway_point; + double halfway_point = 0.0; double accum = 0.0; for (int i = 0; i < N; i++){ accum += samples[i].p_x; diff --git a/src/TheRestOfYourLife/integrate_x_sq.cc b/src/TheRestOfYourLife/integrate_x_sq.cc index cff8f889..dafc7b04 100644 --- a/src/TheRestOfYourLife/integrate_x_sq.cc +++ b/src/TheRestOfYourLife/integrate_x_sq.cc @@ -25,8 +25,6 @@ double pdf(double x) { } int main() { - int inside_circle = 0; - int inside_circle_stratified = 0; int N = 1; auto sum = 0.0; diff --git a/src/TheRestOfYourLife/pi.cc b/src/TheRestOfYourLife/pi.cc index 5f03eb37..18de8a84 100644 --- a/src/TheRestOfYourLife/pi.cc +++ b/src/TheRestOfYourLife/pi.cc @@ -38,7 +38,6 @@ int main() { } } - auto N = static_cast(sqrt_N) * sqrt_N; std::cout << std::fixed << std::setprecision(12); std::cout << "Regular Estimate of Pi = " << (4.0 * inside_circle) / (sqrt_N*sqrt_N) << '\n';