Code cleanup to pass gcc -Weffc++ warnings

[lelegard]

Followup to issue #616

No semantic changes, just added a few missing member initializers in constructors.

[Dmitry-Me]

Those are not "missing", just default constructors are used which is enough.

[lelegard]

@Dmitry-Me : All right, they are "missing to pass -Weffc++ checks". OK with that wording?

In the specific cases of the added initializers, all affected fields had default initializers. So, the code had no bug and the fixes do not change the behavior (hence my comment: "no semantic changes"). However, the initializer warnings are useful if, sometime in the future, an int or pointer field is added without corresponding initializer. We all make mistakes sometimes, don't you? In that case, there will be a bug which is not flagged by gcc with the default options. This is why -Weffc++ is useful. The price to pay is to add initializers for all members, even those with default constructors. Not an expensive price for the added security.

Let's focus on the quality and security of the code, not on the wordings of the comments.

[Dmitry-Me]

Yeah, sure, "quality" of code. It should be an "on by default" warning about a missing built-in type initializer, not a magic "first find it exists" warning complaining about an initializer not being listed explicitly.

What was the warning about copying not supported and not prohibited in XMLPrinter?

[lelegard]

@Dmitry-Me : The warning was:

In file included from tinyxml2.cpp:24:0:
tinyxml2.h:2132:20: error: ‘class tinyxml2::XMLPrinter’ has pointer data members [-Werror=effc++]
 class TINYXML2_LIB XMLPrinter : public XMLVisitor
                    ^~~~~~~~~~
tinyxml2.h:2132:20: error:   but does not override ‘tinyxml2::XMLPrinter(const tinyxml2::XMLPrinter&)’ [-Werror=effc++]
tinyxml2.h:2132:20: error:   or ‘operator=(const tinyxml2::XMLPrinter&)’ [-Werror=effc++]

This is quite valid. If the application copies an XMLPrinter object somehow (assigment or copy constructor), the two objects will point to the same external object. Depending on the class, this can be a problem or not. Typically, if the pointed object is an internal resource (not the case in XMLPrinter) which is freed in the destructor, this is a bug. This is the purpose of the warning.

A proper and larger fix could be to provide a copy constructor and an assignment operator which manage the pointed object properly. In the case of XMLPrinter, what should be done with the FILE* is still to be defined. Shall we use the same external FILE? Shall we disable it in the cloned object? It is up to the author to define and document the semantics. Until then, it makes little sense to clone an object without a proper definition of the semantics of the operation.

Disabling the two operations is a conservative approach. I leave to the author the decision of defining the exact behavior and update the implementation if necessary.

[Dmitry-Me]

Yeah, the warning about copying is good. When the intention is to prohibit copying it's better to have operator= return void so that it's as useless as possible.

[Dmitry-Me]

Do you like LongFitsIntoSizeTMinusOne template struct?

[lelegard]

Interesting. It took me a couple of minutes to understand it and especially why it needed to be inside a struct. Clever trick. The comment is definitely necessary and would probably deserve even more explanations.

[Dmitry-Me]

I pointed out LongFitsIntoSizeTMinusOne struct because to me it's an excellent example of how bad compiler warnings may be.

[lelegard]

Nothing is perfect. But my experience shows that compiler warnings bring an awful lot more benefits than problems. I always use the most paranoid warning mode and turn warnings into errors in all my developments (see the updated makefile in my pull request). And when there is an annoying warning, I address it properly. I must admit, however, that the one that needed LongFitsIntoSizeTMinusOne is a pathological one :)

[Dmitry-Me]

LOL. Visual C++ emits a gazillion warnings with /Wall when compiling Windows SDK headers. That's all you need to know about using most paranoic modes.

[lelegard]

That's all you need to know about using most paranoic modes.

Nope. All I need to know is that the absolute most paranoid warning modes are useful and efficient with gcc on Linux and clang on macOS.

MS VC++ will remain MS VC++. I never considered it as a reference. For the exact reason you mention, I use /W4 "only" on this second-class platform.

[leethomason]

Sorry to be late to this (and resurrect an earlier point) but the changes look good to me except the initializing members with default constructors. That's a lot of clutter. Can that be disabled?

[lelegard]

Hi.

I do not think that this can be disabled. More precisely, I do not think it is possible to make gcc warn about missing initialisers for elementary types and shut up about types with default constructors.

Between the two, no warning about missing essential initialisers / warning about all missing initialisers, the second is much more valuable. It has a cost, those "clutter", but it is cheap compared to unflagged plain bugs which may cause a malfunction.

I use this in all my code.

[Dmitry-Me]

IMO this is the time when compiler warnings are mostly noise. Cppcheck can warn about missing initializers for built-in types and that's indeed valuable. When you're forced to list every initializer even if it's identical to what the compiler would do by default it looks a bit overkill.

[lelegard]

IMO ... it looks a bit overkill.

This is a subjective opinion (and honestly I share it).

As an engineer, I only consider facts and values. Fact: there is no selective warning for built-in types only. Value: flagging missing initialisers for built-in type is valuable. So, I use the available options for their value, I pay the (cheap) price for it and I don't argue too long about what could be done if the compiler and the world were better.

[leethomason]

I've been waffling on this: I get the problem when you use a library that doesn't meet the warning level of the rest of the project and you get inactionable warning spew. But the default members are just too much noise. I'll take the rest of the PR fixes - it's nice cleanup - but the default member constructors are a little too much.

[lelegard]

Hi,

Thanks for taking care about this.

Beyond any aesthetic debate about which constructor is nice and which is ugly, there is only one simple rational question: Is TinyXML-2 made for insertion in source form in any other project or is it a classical library which is separately compiled and used in binary form?

If it is the latter, so far so good, it is your code, your choice.

If it is the former, then it is inconsistent to make a deliberate choice which prevents the code from being compiled within a project with stringent warning constraints (especially using the most widely used compiler on the most widely used architecture, namely gcc on Intel).

You are free to select the objectives of your library, but please stay consistent with that decision.

I went through all the required modifications for TinyXML-2 to be compliant with full warning mode. I will not make the same work every time. So, if each future version of TinyXML-2 needs to be modified, I will simply keep version 5.0.1 forever and fix bugs myself, if I find any in the future (which I doubt about since the code seems good quality overall). In other words, it becomes a fork...

Thanks anyway for your work on TinyXML-2 !

[leethomason]

@lelegard The goal is source inclusion and I fundamentally agree with the point. However, it's not that absolute: 1) compilers conflict on warning messages (LongFitsIntoSizeTMinusOne), 2) it needs to be reasonably maintainable and not too "fiddly". Default method constructors are really fiddly - if I saw that in code (without the effective C++) context I would have no idea why someone would do that. (In fact, honestly, it looks ridiculous) And someone submitting without the gcc/clang toolchain would never see that. (The online build would catch it, but even then you would never expect that.)

It's actually pretty hard for me to get a sense of the sum of the issue. We are 3 people are talking on this thread and there are thousands of users.

I'll try it on VS and see how that compiler reacts.

[lelegard]

About VS, as @Dmitry-Me mentioned in a previous comment, /Wall is almost unusable, not because of our code but simply because the system headers do not comply with /Wall ! So, the experience shows that VS is not a reference platform, in almost no circumstances. The only added value I found to VC /W4 (the highest warning level which doesn't kill the system headers) is the discovery of a few potential data losses such as assigning an uint32_t into an uint8_t for instance, which were not flagged by gcc.

Anyway, I won't pollute this thread with any additional comment beyond this one. Before exiting, I just would like to emphasise two points:

1. Since you acknowledge that source inclusion is a goal, you must comply with all valid combinations of options of any common compiler. Otherwise, your goal won't be credible. This may lead to "fiddly" code, but this is very common when you address portability, whether it is across platforms, compilers or sets of compilation options. Moreover, there is no additional work for you, I already did it in the pull request. So, removing the initialisers is indeed more work for you :) and this will be considered as intentional work which breaks the portability between sets of compiler options.

2. You wrote "We are 3 people are talking on this thread and there are thousands of users". Do not forget that -Weffc++ is defined as the implementation of Scott Meyers' recommendations from "Effective C++". Scott Meyers is a renowned C++ guru and his series of books is the bible of C++ developers. Many C++ developers will find quite presumptuous to despise Meyers' recommendations and deliberately avoid them. The first point was about TinyXML's credibility. This one is clearly about yours.

OK, now I leave you in peace about this subject. I brought report, code, rational arguments, facts and benefits. I let you do whatever you decide about this point. On my side, for what it's worth, I will continue with the radical set of warning options because the benefits are higher than the costs. Consequently, I keep my modified version of TinyXML-2 in my project (@tsduck). If subsequent versions become compatible with full warnings, I will use them. Otherwise, I will go on with my "tiny little fork".

Thank you all.
Bye.

[leethomason]

This doesn't compile on my OSX machine - it's upset about (long long) being (technically) c++ 11 and format strings not be literals. On the other hand, the code itself seems sound across the platforms.

I'm backed into a corner - I'll have to remove some of the warnings (which get reported as errors.) I'll keep poking at it and submit something tomorrow.

[leethomason]

Looking at the changes "in situ" - not as a diff - the default constructors bother me less. And the other changes are valuable. The effc warnings are useful, but dangerous, so I moved them to the 'effc' target of the Makefile. You can use make effc to build with the very pedantic warnings, but it doesn't break the systems (clang) that conflict.