Fix deprecated copies and redundant moves

[mcm001]

While working on wpilibsuite/allwpilib#7606 I had to disable deprecated copy and redundant move warnings. I'm not super great with this aspect of C++, but I've added default constructors where they were previously implicitly declared.

[varunagrawal]

Doesn't C++ automatically add these if they're not defined? I may be wrong though.

[mcm001]

Yes, but if you have deprecated copy warnings enabled they'll reported warnings. I'm trying to find a primary source, but per gcc's bugzilla,

"The implicit definition of a copy constructor as defaulted is deprecated if the class has a user-declared copy assignment operator or a user-declared destructor. The implicit definition of a copy assignment operator as defaulted is deprecated if the class has a user-declared copy constructor or a user-declared destructor"

TLDR: compile with -Wdeprecated-copy and observe compiler output

[mcm001]

Although this might be worth a larger discussion. What warnings does GTSAM currently compile with? Is there any interest in enabling more compiler warnings/enabling Werror?

[varunagrawal]

Okay I'm going to need your help understanding this better. I currently don't get these warnings so is it a result of a newer version of GCC?

In general we ignore warnings since most of the time they don't reflect a real problem. I'm curious if suppressing these warnings are sufficient in your use case?

My guiding philosophy here is less code is good code.

[dellaert]

In general we ignore warnings since most of the time they don't reflect a real problem. I'm curious if suppressing these warnings are sufficient in your use case?

Disagree. If we can -Wall so much the better!

My guiding philosophy here is less code is good code.

Totally agree here, but I have a suspicion that this extra code might actually benefit performance even for other compilers? any idea, @mcm001 ?

[mcm001]

Hey! At least in some Godbolt testing, I don't see a meaningful difference in the actual binary output of this contrived test program with GCC 12.2 on x86 or arm64. I think this just comes down to if y'all are in favor of fixing warnings even if the code output is (probably) correct. I think that fixing rule-of-3/5/0-related warnings is a good idea, and deleting redundant moves also lets us write less code ;)

[dellaert]

I’m in favor of -Wall so approving. We should probably add a gcc12 CI, maybe instead of gcc11. Not sure whether gcc9 is already EOL?

[mcm001]

I've also got a tiny patchset that lets me build with -Wall -Wpedantic on GCC 12, should be ready here in a moment. Happy to add that in a follow-on PR, or add to this one

[mcm001]

And from https://gcc.gnu.org/gcc-9/ and https://gcc.gnu.org/gcc-10/ , it appears GCC 9/10 are EOL yeah

[dellaert]

Awesome ! Maybe do it in a follow-up PR where you also switch 11 to 12 in CI?
I know some folks were using gcc9 on some embedded platforms but maybe we could switch that to 10…

[mcm001]

Sounds good! Will add in a follow-on