PVS-Studio Team: Top 10 bugs in C++ open source projects, checked in 2016

[Andrey Karpov]

While the world is discussing the 89th Ceremony of Oscar award and charts of actors and costumes, we've decided to write a review article about the IT-sphere. The article is going to cover the most interesting bugs, made in open source projects in 2016. This year was remarkable for our tool, as PVS-Studio has become available on Linux OS. The errors we present are hopefully, already fixed, but every reader can see how serious are the errors made by developers.

Top 10 bugs in C++ open source projects, checked in 2016: https://www.viva64.com/en/b/0483/

[Richard]

[Please do not mail me a copy of your followup]

Andrey Karpov <karpo...@gmail.com> spake the secret code
<96e360cf-fb29-428c...@googlegroups.com> thusly:

>Top 10 bugs in C++ open source projects, checked in 2016:
>https://www.viva64.com/en/b/0483/

I don't see the problem in the 5th place entry. The code excerpt
looks like this:

class G4PhysicsModelCatalog
{
private:
....
G4PhysicsModelCatalog();
....
static modelCatalog* catalog;
....
};

G4PhysicsModelCatalog::G4PhysicsModelCatalog()
{ if(!catalog) {
static modelCatalog catal;
catalog = &catal;
}
}

G4int G4PhysicsModelCatalog::Register(const G4String& name)
{
G4PhysicsModelCatalog();
....
}

Now there is something missing here to allow me to completely
understand this snippet out of context. We don't see the static
initializer for the catalog member. Presumably it looks like

modelCatalog *G4PhysicsModelCatalog::catalog = nullptr;

It seems the call to the constructor in the final function snippet is
intended to get the static member catalog initialized. You are
correct in that the code as written constructs a temporary object and
then destroys it, but it also invokes the constructor body which will
initialize this static member if it isn't already initialized.

What exactly is the problem here?

I agree with your recommendations for a better design that explicitly
reveals the intent (initialize the static members to a well known
state; again static class members are just another kind of global
variable). However, other than a clunky design, I'm not seeing any
bugs here.
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Terminals Wiki <http://terminals-wiki.org>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>

[Andrey Karpov]

> I don't see the problem in the 5th place entry.

You're right, it's a slip-up. It happens, you know. Only those who don't write articles don't make mistakes in them. :)

However, it's shit code anyway and one shouldn't write like that. Perhaps the programmer wanted to call the constructor but failed, and the code works only because of sheer luck.

[Rick C. Hodgin]

On Sunday, March 12, 2017 at 2:43:54 PM UTC-4, Andrey Karpov wrote:
> > I don't see the problem in the 5th place entry.
> You're right, it's a slip-up. It happens, you know. Only those who don't
> write articles don't make mistakes in them. :)
>

> However, it's .. code anyway...

Andrey Karpov, do you think your post was better with the profanity in
there?

Has anyone ever taught you who Jesus is? What He's about? What sin is?
And why you need to be forgiven for your sin?

Thank you,
Rick C. Hodgin

[Manfred]

Not sure that the code works only because of sheer luck.
In fact any access to G4PhysicsModelCatalog is guaranteed to have the
(private static) catalog initialized, so the code appears to work, and
not by coincidence.
Still the design could be improved.

[Richard]

[Please do not mail me a copy of your followup]

Andrey Karpov <karpo...@gmail.com> spake the secret code

<98c04023-1741-4202...@googlegroups.com> thusly:

Yeah, I totally agree that the design is awkward. If your intention
is to initialize the globals-variables-declared-as-class-static-members,
then make that intention clear by an appropriately named static
funtion like

prepareCatalog()