Making sure that the library can compile without warnings even when crazy pedantic flags are set

[lemire]

We use cxxopts in simdjson since some of our users get upset if they get warnings (sometimes calling these warnings 'bugs').

We can disable the warnings on our end, but it might be nicer for everyone if cxxopts was also "warning free". This PR should help.

It does two things:

• It adds the "{}" suffix to class-type attributes that are not explicitly initialized by all constructors. This effectively makes sure that they are initialized with a default constructor.
• It disables some specific GCC warning for the duration of the definition/declaration of a class.

None of these changes should impact the performance or the functionality of the library.

Note that it is not my claim that I am fixing defects. This is purely to save time in the long term by not getting warnings.

cc @jkeiser

---

This change is 

[eyalroz]

@lemire : can you list the "crazy pedantic flags" you're using in your compilation?

[lemire]

@eyalroz

can you list the "crazy pedantic flags" you're using in your compilation?

Sure. We compile with -Werror -Wall -Wextra -Weffc++ -Wsign-compare -Wshadow -Wwrite-strings -Wpointer-arith -Winit-self -Wconversion -Wno-sign-conversion.

[lemire]

Or (visual studio): /WX /W3 /sdl /w34714.

[lemire]

The issues here are raised by -Weffc++ under GNU GCC.

This not a flag that I would normally use, but some of our users do like this flag.

[eyalroz]

Thanks for the set of switches.

However - I think I disapprove of your warning suppressions. If you were to resolve the issue which triggers the warning, that's fine, but if you're not doing that - suppression doesn't seem like the "right thing" to do. Would it not be more reasonable for your own code to apply the warning suppression right before you include cxxopts.hpp?

BTW - This warning with enable_shared_from_this is puzzling. I've just asked this question on StackOverflow.

[lemire]

@eyalroz Currently, using cxxopts involves this code routine...

SIMDJSON_PUSH_DISABLE_ALL_WARNINGS
#ifndef __cpp_exceptions
#define CXXOPTS_NO_EXCEPTIONS
#endif
#include "cxxopts.hpp"
SIMDJSON_POP_DISABLE_WARNINGS

This is rather inconvenient, as you can see. We are piling up code just to be able to use cxxopts.

Let me put it less nicely: it is ugly as hell.

If you were to resolve the issue which triggers the warning, that's fine, but if you're not doing that - suppression doesn't seem like the "right thing" to do.

Is there even an issue to fix? Can you explain it to me? I'd love to understand why there is a real issue in need of fixing.

My own diagnostic is that the code is fine. When this happens, it is perfectly legitimate to silence the warning.

I did not just silence the warning. I looked at the code, concluded that there was no problem.

Now... I might be wrong. If I am, please explain the mistake to me. I will eagerly update my PR.

As for the larger issue...

I think that the "issue" is that -Weffc++ should NOT be normally used. Or, at least, people who use it should be expect their code to be free from warnings.

But here is the thing... there are people who won't listen. They will insist to believe that if there is a warning, then there must be a mistake, and that the mistake needs to be fixed. Even if you tell them that there is no "bug" they will insist that you fix your code.

So that's where silencing the warning is a good social patch.

They compile their code... no warning... they are happy. Everyone moves on.

[eyalroz]

So, I'm convinced that it's reasonable to have the no-virtual-dtor suppression. (Not that I have any say in the matter of course...)

But - it's actually not your argument that convinced me, rather the fact that there isn't a reasonable way to avoid the warning other than suppressing it.

(In your case - you could simply have a wrapper include, which has that ugly snippet just once, and use that one instead of repeating the snippet every time you need the options. It makes more sense to add that to your project than to place more code in cxxopt.hpp itself. But like I said - the suppression is justifiable in itself.)

[lemire]

@eyalroz

it's actually not your argument that convinced me

Which of my arguments you did not find convincing? I had a few: (1) people should not use -Weffc++ and expect warning-free compilations, (2) there is no issue to fix, it is a bad static analysis, (3) it is ugly to have to silence warnings when you are including cxxopts, (4) we really should not bother silencing these warnings, but it is less work ultimately than explaining to folks that "no, there is no actual error".

[lemire]

Note that for the record, I am not happy with these ugly warning silencing routines, and I probably share much of @eyalroz's sentiment.

This whole thing is unpleasant.

It is a tradeoff ultimately.

[eyalroz]

@lemire :

1. I agree with this, but if people shouldn't use it - why should cxxopts accommodate this use?
2. It's not really bad static analysis, in the sense that you can theoretically delete an enabled_shared_from_this. Of course nobody does that - but it's a convention, not an impossibility. If it were somehow prevented, then I would agree.
3. Having to silence warnings is ugly, but it's uglier if all users of cxxopts have to bear this ugliness even if they don't use -Weffc++ (which we agreed they shouldn't and probably aren't).
4. What should really happen IMHO is for this warning not to be emitted, by somehow changing enabled_shared_from_this. So it is a valid warning. But of course, we can't just change the standard library, and this is why silencing is legitimate IMHO.

Apologies to everyone reading this page for wasting your time with nitpicking :-(

[jarro2783]

I'm ok with this change, but it will need rebasing.

[lemire]

@jarro2783 I have rebased.

[lemire]

@jarro2783 My rebasing exposed an issue whereas the function signature of AppendString varied depending on the macros. Fixed.

I recommend you have a look at my PR. It should make the library more usable by more people.

[jarro2783]

Looks good, I'll merge it.