Match default compiler warning flags to Gaffer

[danieldresser-ie]

I've tried to also match the fixes to how things were fixed in Gaffer.

The most possibly contentious is probably switching to defaulted copy/assignment - maybe some of these cases are philosophical special enough that they deserve user implementation, despite that user implementation being identical to the default? My reason for using default is that in all these cases we were relying on default behaviour for either copying or assignment.

I also explicitly defaulted the destructors ... not quite sure why, but it matches what you did in Gaffer.

[danieldresser-ie]

Hmm, dealing with compiler flags seems a little more complex than I expected, because of cross platform issues ... originally, I was only setting the gcc flags on posix, so I fixed that so that I also set -Wno-unused-parameter on Mac ... and now it can't find boost? Not sure yet if those are somehow actually related, or if the CI is just a bit accursed.

On Windows the problem seems more straightforward ... it doesn't support [[fallthrough]] unless it's told to use the C++17 standard? GCC seems to support [[fallthrough]] with C++14 ... can we just tell it to use C++17? Seems a bit more aggressive than I expected to need to be here, I'm just trying to get our builds working at IE

[johnhaddon]

LGTM.

... and now it can't find boost?

The config checks work by trying to build a small program, so the most plausible cause of failure is that clang doesn't like the new command line flags. In the Gaffer CI, we have a handy little post-task that helps debug this sort of thing, by printing out the contents of config.log :

https://github.com/GafferHQ/gaffer/blob/main/.github/workflows/main.yml#L255-L266

Would be worth adding that to Cortex I think.

can we just tell it to use C++17?

We really should be using C++17 already - it's mandated by VFXPlatform, and that's what we're building Gaffer with. The problem with it is that we've still got legacy uses of random_shuffle, which is deprecated in C++14 and removed in C++17. GCC/libstdc++ hasn't actually actioned that, but Clang/libc++ has. Let me see if I can get something together for that - I was looking at it the other day for my M1 builds anyway.

[johnhaddon]

I've merged my C++17 PR, including the debug step for failed builds. If you rebase this PR on main now, it should either pass or give us information in the log about why it's failing to find Boost on Mac.

[danieldresser-ie]

OK, tests are passing now ( aside from a timing test running a bit long on debug ).

Really not sure what's going on with gcc on darwin ... -Wno-cast-function-type should be available on any gcc since 8 ... not sure the best way to check the gcc version on mac, but when I added a test for compilerVersion >= 8, it took that branch. Maybe just omitting the flag on darwin is fine?

For the error print, I'm currently printing both config.log and buildConfig.log ... perhaps messy, but seems to work fine.

[johnhaddon]

Really not sure what's going on with gcc on darwin ... -Wno-cast-function-type should be available on any gcc since 8 ... not sure the best way to check the gcc version on mac, but when I added a test for compilerVersion >= 8, it took that branch.

I think the issue is that by default on Mac, g++ is really clang++, and reports the Clang version, not the GCC version (currently 13 on my Mac). Rather than fix it with the extra platform == "posix" check you added, I think we should instead default CXX to clang++ on Mac like we do for Gaffer : https://github.com/GafferHQ/gaffer/blob/main/SConstruct#L82-L86. Then our if CXX=="g++" conditionals will only be used when the compiler is truly GCC.

[danieldresser-ie]

Ah, OK, that makes a lot more sense. I wasn't aware that g++ wasn't g++

[danieldresser-ie]

OK, all passing again except for the unrelated timing issue with debug. I've squashed that part. Worth reviewing: the commit where I also print config.log, and the final commit where I fix the compile flags ... that one is a bit weird.

We're using g++" in os.path.basename( env["CXX"] ) to test for g++ ... but that also matches clang++. I've added tests that it matches g++ and not clang++, which seems to match what Gaffer does, but it feels weird that we can't come up with a name to use exact comprison on, rather than substring matching.