Lint pass: ctor initializer order + virtual dtors #865

hollasch · 2021-02-14T03:56:40Z

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.

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.

trevordblack

Should I add warnings to the CMAKE for clang compilation?

trevordblack

Seems mostly positive.

All of these book changes will need diligent checking after the fact.
But, at least for today, virtual destructors was probably overdue

hollasch · 2021-02-19T09:03:14Z

Yes, add clang flags as appropriate. Append to this branch.

hollasch · 2021-02-19T09:03:29Z

Oops, forgot to @trevordblack .

trevordblack · 2021-02-20T22:14:39Z

Couple of questions:

Should I enable all warnings, or only those that match their MSVC counterparts?
We don't want to set all warnings as errors?

trevordblack · 2021-02-20T22:20:45Z

Getting up and going for both clang and gcc is trivial:

if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
    ...
else ()
    add_compile_options(-Wall -Wextra -pedantic -Werror) # all warnings for both clang and g++
endif()

But clang and gcc have different warnings (this also varies across versions, yay!)

I think there's value in having the superset of all MSVC, clang, and gcc warnings spit out to you and me, but I think it would be really annoying/confusing/discouraging for new comers.

I'm saying we should make a build farm.
Let's make a build farm
Let's make a build farm.

trevordblack · 2021-02-21T19:26:36Z

With -Wall -Wextra -pedantic in clang6

trevordblack@DESKTOP-7G6O9AE:/mnt/d/raytracing.github.io/build_clang6$ make
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/estimate_halfway.cc:53:21: warning: variable 'halfway_point' is used uninitialized whenever 'for' loop exits because its condition is false
      [-Wsometimes-uninitialized]
    for (int i = 0; i < N; i++){
                    ^~~~~
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/estimate_halfway.cc:64:34: note: uninitialized use occurs here
    std::cout << "Halfway = " << halfway_point << '\n';
                                 ^~~~~~~~~~~~~
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/estimate_halfway.cc:53:21: note: remove the condition if it is always true
    for (int i = 0; i < N; i++){
                    ^~~~~
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/estimate_halfway.cc:51:25: note: initialize the variable 'halfway_point' to silence this warning
    double halfway_point;
                        ^
                         = 0.0
1 warning generated.
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/sphere_importance.cc:25:24: warning: unused parameter 'd' [-Wunused-parameter]
double pdf(const vec3& d) {
                       ^
1 warning generated.
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/pi.cc:41:10: warning: unused variable 'N' [-Wunused-variable]
    auto N = static_cast<double>(sqrt_N) * sqrt_N;
         ^
1 warning generated.
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/integrate_x_sq.cc:19:17: warning: unused parameter 'd' [-Wunused-parameter]
double f(double d) {
                ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/integrate_x_sq.cc:28:9: warning: unused variable 'inside_circle' [-Wunused-variable]
    int inside_circle = 0;
        ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/integrate_x_sq.cc:29:9: warning: unused variable 'inside_circle_stratified' [-Wunused-variable]
    int inside_circle_stratified = 0;
        ^
3 warnings generated.
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/cos_cubed.cc:18:17: warning: unused parameter 'r1' [-Wunused-parameter]
double f(double r1, double r2) {
                ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/cos_cubed.cc:26:19: warning: unused parameter 'r1' [-Wunused-parameter]
double pdf(double r1, double r2) {
                  ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/cos_cubed.cc:26:30: warning: unused parameter 'r2' [-Wunused-parameter]
double pdf(double r1, double r2) {
                             ^
3 warnings generated.
...
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/hittable.h:47:42: warning: unused parameter 'o' [-Wunused-parameter]
    virtual double pdf_value(const vec3& o, const vec3& v) const {
                                         ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/hittable.h:47:57: warning: unused parameter 'v' [-Wunused-parameter]
    virtual double pdf_value(const vec3& o, const vec3& v) const {
                                                        ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/hittable.h:51:37: warning: unused parameter 'o' [-Wunused-parameter]
    virtual vec3 random(const vec3& o) const {
                                    ^
...
/mnt/d/raytracing.github.io/src/common/texture.h:36:24: warning: unused parameter 'u' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                       ^
/mnt/d/raytracing.github.io/src/common/texture.h:36:34: warning: unused parameter 'v' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                 ^
/mnt/d/raytracing.github.io/src/common/texture.h:36:51: warning: unused parameter 'p' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                                  ^
/mnt/d/raytracing.github.io/src/common/texture.h:80:24: warning: unused parameter 'u' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                       ^
/mnt/d/raytracing.github.io/src/common/texture.h:80:34: warning: unused parameter 'v' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                 ^
/mnt/d/raytracing.github.io/src/common/texture.h:95:51: warning: unused parameter 'p' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                                  ^
In file included from /mnt/d/raytracing.github.io/src/TheRestOfYourLife/main.cc:19:
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:34:20: warning: unused parameter 'r_in' [-Wunused-parameter]
        const ray& r_in, const hit_record& rec, double u, double v, const point3& p
                   ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:34:44: warning: unused parameter 'rec' [-Wunused-parameter]
        const ray& r_in, const hit_record& rec, double u, double v, const point3& p
                                           ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:34:56: warning: unused parameter 'u' [-Wunused-parameter]
        const ray& r_in, const hit_record& rec, double u, double v, const point3& p
                                                       ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:34:66: warning: unused parameter 'v' [-Wunused-parameter]
        const ray& r_in, const hit_record& rec, double u, double v, const point3& p
                                                                 ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:34:83: warning: unused parameter 'p' [-Wunused-parameter]
        const ray& r_in, const hit_record& rec, double u, double v, const point3& p
                                                                                  ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:39:37: warning: unused parameter 'r_in' [-Wunused-parameter]
    virtual bool scatter(const ray& r_in, const hit_record& rec, scatter_record& srec) const {
                                    ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:39:61: warning: unused parameter 'rec' [-Wunused-parameter]
    virtual bool scatter(const ray& r_in, const hit_record& rec, scatter_record& srec) const {
                                                            ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:39:82: warning: unused parameter 'srec' [-Wunused-parameter]
    virtual bool scatter(const ray& r_in, const hit_record& rec, scatter_record& srec) const {
                                                                                 ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:43:46: warning: unused parameter 'r_in' [-Wunused-parameter]
    virtual double scattering_pdf(const ray& r_in, const hit_record& rec, const ray& scattered)
                                             ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:43:70: warning: unused parameter 'rec' [-Wunused-parameter]
    virtual double scattering_pdf(const ray& r_in, const hit_record& rec, const ray& scattered)
                                                                     ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:43:86: warning: unused parameter 'scattered' [-Wunused-parameter]
    virtual double scattering_pdf(const ray& r_in, const hit_record& rec, const ray& scattered)
                                                                                     ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:55:29: warning: unused parameter 'r_in' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, scatter_record& srec) const override {
                            ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:62:38: warning: unused parameter 'r_in' [-Wunused-parameter]
    double scattering_pdf(const ray& r_in, const hit_record& rec, const ray& scattered)
                                     ^
/mnt/d/raytracing.github.io/src/TheRestOfYourLife/material.h:137:30: warning: unused parameter 'r_in' [-Wunused-parameter]
    color emitted(const ray& r_in, const hit_record& rec, double u, double v, const point3& p)
                             ^
23 warnings generated.
...
/mnt/d/raytracing.github.io/src/common/texture.h:36:24: warning: unused parameter 'u' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                       ^
/mnt/d/raytracing.github.io/src/common/texture.h:36:34: warning: unused parameter 'v' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                 ^
/mnt/d/raytracing.github.io/src/common/texture.h:36:51: warning: unused parameter 'p' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                                  ^
/mnt/d/raytracing.github.io/src/common/texture.h:80:24: warning: unused parameter 'u' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                       ^
/mnt/d/raytracing.github.io/src/common/texture.h:80:34: warning: unused parameter 'v' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                 ^
/mnt/d/raytracing.github.io/src/common/texture.h:95:51: warning: unused parameter 'p' [-Wunused-parameter]
    color value(double u, double v, const point3& p) const override {
                                                  ^
In file included from /mnt/d/raytracing.github.io/src/TheNextWeek/main.cc:18:
In file included from /mnt/d/raytracing.github.io/src/TheNextWeek/constant_medium.h:17:
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:24:34: warning: unused parameter 'u' [-Wunused-parameter]
    virtual color emitted(double u, double v, const point3& p) const {
                                 ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:24:44: warning: unused parameter 'v' [-Wunused-parameter]
    virtual color emitted(double u, double v, const point3& p) const {
                                           ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:24:61: warning: unused parameter 'p' [-Wunused-parameter]
    virtual color emitted(double u, double v, const point3& p) const {
                                                            ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:118:29: warning: unused parameter 'r_in' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered)
                            ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:118:53: warning: unused parameter 'rec' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered)
                                                    ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:118:65: warning: unused parameter 'attenuation' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered)
                                                                ^
/mnt/d/raytracing.github.io/src/TheNextWeek/material.h:118:83: warning: unused parameter 'scattered' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered)
                                                                                  ^
13 warnings generated.
...
/mnt/d/raytracing.github.io/src/InOneWeekend/material.h:34:29: warning: unused parameter 'r_in' [-Wunused-parameter]
    bool scatter(const ray& r_in, const hit_record& rec, color& attenuation, ray& scattered)
                            ^
1 warning generated.
[100%] Linking CXX executable inOneWeekend
make[2]: warning:  Clock skew detected.  Your build may be incomplete.
[100%] Built target inOneWeekend

trevordblack · 2021-02-21T19:32:37Z

With -Wall -Wextra -pedantic in gcc7.5 (skimming for brevity)`

lots of -Wunused-parameter
lots of -Wunused-variable
lots of -Wmisleading-indentation:

/mnt/d/raytracing.github.io/src/common/external/stb_image.h: In function ‘unsigned char* stbi__convert_format(unsigned char*, int, int, unsigned int, unsigned int)’:
/mnt/d/raytracing.github.io/src/common/external/stb_image.h:1338:44: warning: this ‘for’ clause does not guard... [-Wmisleading-indentation]
       #define CASE(a,b)   case COMBO(a,b): for(i=x-1; i >= 0; --i, src += a, dest += b)
                                            ^
/mnt/d/raytracing.github.io/src/common/external/stb_image.h:1342:10: note: in expansion of macro ‘CASE’
          CASE(1,2) dest[0]=src[0], dest[1]=255; break;
          ^~~~
/mnt/d/raytracing.github.io/src/common/external/stb_image.h:1342:49: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘for’
          CASE(1,2) dest[0]=src[0], dest[1]=255; break;
                                                 ^~~~~

This thing:

/mnt/d/raytracing.github.io/src/common/external/stb_image.h: In function ‘void stbi__build_fast_ac(stbi__int16*, stbi__huffman*)’:
/mnt/d/raytracing.github.io/src/common/external/stb_image.h:1557:36: warning: left shift of negative value [-Wshift-negative-value]
             if (k < m) k += (-1 << magbits) + 1;

trevordblack · 2021-02-21T19:34:20Z

I also tried -Wall -Wextra -pedantic in gcc10.1 and found similar results to gcc7.5 (didn't look too closely)

trevordblack · 2021-02-21T19:39:51Z

That stb warning seems to have been slightly rewritten in a later release:

            if (k < m) k += (~0U << magbits) + 1;

Making that change locally removes the warning

trevordblack · 2021-02-21T19:40:48Z

Having gcc and clang spit out the same warnings as msvc requires:

if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
    ...
else ()
    add_compile_options(-Wnon-virtual-dtor) # Class has virtual functions, but its non-trivial destructor is not virtual
    add_compile_options(-Wreorder) # Data member will be initialized after [other] data member
endif()

hollasch · 2021-02-21T23:10:11Z

Ugh. Some thoughts:

I have to deal with the vagaries of clang and gcc on Mac at work as well. Every version's different, including std library implementations (I was messed up a couple of weeks ago because our version did have std::nextafter(double)). Probably good that you made this effort, because I agree with you -- it's a miserable road starting to maintain a billion different compiler flag sets. And yeah, this will change from one MSVC version to another as well. So we should pull such flags.

Still, this is useful. As a one-off manual exercise, sure, but it would be nice to also have a consistent build set for validation of externally submitted PRs. And yet, I don't want to have to spool up builds on my Mac and run on the X different platforms for every change. I'm thinking that we should just do this ad hoc as we've been doing. If tools reveal a flaw, we'll just fix the flaw. I agree with you that we should keep the build tools dead simple for readers.

I also don't care at all about warnings coming out of stb_image.h. That's basically a third-party library, and if their code is unclean in some way, I don't really care as long as our specific use case gets legit results.

We do have a bunch of unused parameters, but that's unhelpfully fussy in my opinion. This is a good example of why some warnings should be optional. We could go with the pattern of void func(bool thing, int /* count */, char /* letter */), where if you comment out the parameter name you avoid this error. But I wouldn't be surprised if we don't use parameters at some stage, only to use the later in development. That would be a real pain commenting and uncommenting method parameters to satisfy this warning.

Unused/uninitialized variables we should definitely fix.

hollasch · 2021-02-21T23:12:48Z

Oh, another thing. I do think the MSVC compiler flags are nice in the CMakeLists.txt as a way to show how to set compiler flags, for those (like me) who are very green about CMake. We could just keep it as an illustrative example that actually works. Perhaps that and some generic clang/gcc compiler. I don't think it's safe to assume that if you're not using MSVC, you must be using gcc or clang.

trevordblack · 2021-02-21T23:19:40Z

I 100% agree with everything you've said.

hollasch · 2021-02-21T23:21:32Z

Ok, so keep the current MSVC flags and add some general clang / gcc and stop there, yes? If so, go ahead and add the flags you recommend and tag on another commit.

trevordblack · 2021-02-21T23:28:48Z

okee doke.

hollasch · 2021-02-22T18:47:48Z

Thanks! Fun fact -- I can't approve, because I authored some of the commits, and presumably neither can you, for the same reason. Oh well.

hollasch added this to the v4.0.0 milestone Feb 14, 2021

hollasch requested a review from trevordblack February 14, 2021 03:56

trevordblack reviewed Feb 19, 2021

View reviewed changes

trevordblack approved these changes Feb 19, 2021

View reviewed changes

trevordblack added 2 commits February 22, 2021 00:37

Added warnings for clang and gnu to CMake

2c29520

Cleaned up warnings found in clang and gnu

944fd9b

hollasch merged commit 9540eea into dev-major Feb 22, 2021

hollasch deleted the lint-pass branch February 22, 2021 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lint pass: ctor initializer order + virtual dtors #865

Lint pass: ctor initializer order + virtual dtors #865

hollasch commented Feb 14, 2021

trevordblack left a comment

trevordblack left a comment

hollasch commented Feb 19, 2021

hollasch commented Feb 19, 2021

trevordblack commented Feb 20, 2021 •

edited

Loading

trevordblack commented Feb 20, 2021 •

edited

Loading

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 21, 2021

hollasch commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 22, 2021

Lint pass: ctor initializer order + virtual dtors #865

Lint pass: ctor initializer order + virtual dtors #865

Conversation

hollasch commented Feb 14, 2021

trevordblack left a comment

Choose a reason for hiding this comment

trevordblack left a comment

Choose a reason for hiding this comment

hollasch commented Feb 19, 2021

hollasch commented Feb 19, 2021

trevordblack commented Feb 20, 2021 • edited Loading

trevordblack commented Feb 20, 2021 • edited Loading

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 21, 2021

hollasch commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 21, 2021

trevordblack commented Feb 21, 2021

hollasch commented Feb 22, 2021

trevordblack commented Feb 20, 2021 •

edited

Loading

trevordblack commented Feb 20, 2021 •

edited

Loading