Recurring C++ and Qt anti-patterns

Kent-Dorfman

@chris-kawa said in Recurring C++ and Qt anti-patterns:

It's not a correctness bug. It's a subtle performance one. Compare this with:

It took me a few minutes but ok, I'm convinced... LOL

fcarney

mtime.toString(tr("M/d/yyyy hh:mm AP"));

aha_1980

Hi @fcarney,

mtime.toString(tr("M/d/yyyy hh:mm AP"));

OMG. Yeah, that's a good (bad) one :)

fcarney

Just created this pattern today:

if(condition == somevalue)
somestatement.append(whatever);

I forgot the indentation so it didn't look like and if statement.
I I know this is really simple and not an error/bad practice. It is more a readability issue.

Going to be more rigorous in the future:

if(condition == somevalue){
    somestatement.append(whatever);
}

SGaist

Well, that one made a big security hole in Apple's authentication code but it was the other way around, several lines under the if without curly brackets.

fcarney

Wow, okay, I take it back. Bad pattern!

aha_1980

@SGaist I've seen GCC 7.3 warning about exactly this problem ("thif if clause ... does not guard ...").

So hopefully such problems will go away sooner than later.

Regards

kshegunov

Nope. It warns if it doesn't actually guard (as if you had put incidentally ; at the end of the if):

if (something)
something else;
something else else; //< Can't warn about that

aha_1980

OMG

QString CharToString(char *str)
{
    QString result = "";
    int lengthOfString = strlen(str);

    QString s;
    for(int i = 0; i < lengthOfString; i++)
    {
        s = QString("%1").arg(str[i], 0, 16);

        if(s.length() == 1)
            result.append("0");

        result.append(s);
    }

    return result;
}

There are multiple anti-patterns in that function (who finds all?!), but basically the solution is to use: QString s = QByteArray::toHex(str);

aha_1980

@kshegunov

What I meant was the following example:

which is already an improvement on poorly formatted code.

Regards

fcarney

Including the wrong header that doesn't match cpp file definition... This is really confusing.

fcarney

Hmmm... I have just found some icky syntax that makes me think it is an anti-pattern just cause its icky:

#include <vector>

template <class T>
class IteratorClass
{
public:
    std::vector<int>::iterator end(); // neat syntax
    std::vector<T>::iterator end2(); // error, needs typename
    typename std::vector<T>::iterator begin(); // really? this is getting ugly
    using retIterator = typename std::vector<T>::iterator; // ugly
    typedef typename std::vector<T>::iterator retIterator; // fugly

private:
    std::vector<T> m_data;
};

Got stuck on this last night and just couldn't figure out why the simplest syntax would not work with templates. My IDE even told me I needed "typename" and I kept trying "typedef" (💩). Yeah, it was not a good time to be coding, so I went to bed. 😀

Kent-Dorfman

@fcarney said in Recurring C++ and Qt anti-patterns:

std::vector<T>::iterator end2(); // error, needs typename

Yeah, this being an error (inside a template definition) really bugs me as well.

I'm sure there is a perfectly good brainiac reason it barfs, but I could really see myself wanting to do something like this, as "end2" instead of end2()

fcarney

@Kent-Dorfman
Apparently its a "dependent name":
https://en.cppreference.com/w/cpp/language/dependent_name

I have not taken time to understand it, but there is the "reason".

fcarney

I think I may need to stop coding in the evening. I ran into a weird bug that I cannot duplicate today:

#include <vector>

template<class T>
class SomeObject
{
    using Storage = std::vector<T>;
public:
    SomeObject(size_t len){
        m_data.resize(len);
    }

    size_t getSize(){
        return m_data.size();
    }

private:
    Storage m_data;
};

class UsesSomeObject
{
public:
    UsesSomeObject()
        : m_someval(0)
        , m_somedata(128) // if not initialized the whole object was spitting out weird data
    {

    }

private:
    int m_someval;
    SomeObject<int> m_somedata;
};

I don't know if this had anything to do with templates or not. I was working with one at the time. There is a comment in the above code about not initializing m_somedata. I didn't have a default constructor or maybe it created one for me (not sure). Accessing the vector internal to the class had all sorts of "interesting" behavior. Then when I realized my error everything started working fine. It was just a very sneaky issues. However, on my compiler at work it is not letting me compile this. So I am not sure of the situation where it would let me compile this. Maybe if it creates its own default constructor. The lesson is make sure everything is getting initialized before using them!

I will check tonight to see if I can simplify the actual condition that caused this. It was quite interesting and the errors didn't match the source of the problem.

fcarney

Not an antipattern, just disappointing. I cannot do this:

std::vector<float&> frefs;

I know why. I know you can use std::reference_wrapper, but it is kinda messy to me.

Kent-Dorfman

I guess I don't have a problem with it because in the cases where I might ever consider such an abomination there are always pointers...yes, always pointers.

fcarney

@Kent-Dorfman said in Recurring C++ and Qt anti-patterns:

always pointers

We should start an anti-safe coding movement (I say this with disdain for idea of safety, there is nothing safe about systems level coding IMO, or coding in general) . The slogan would be "Always Pointers".

Chris Kawa

I hate to break this to you guys but pointers and references are the same thing. References are just syntax constraint, something like const, so disappointment in this case would be like disappointment that you can't assign to a const value. IMO using std::reference_wrapper because you don't like those naked stars is just silly.
As for "Always Pointers" - why so extreme? How about more mellow party like "pointers where they make sense"?

fcarney

@Chris-Kawa said in Recurring C++ and Qt anti-patterns:

pointers where they make sense

That is the point (hehe) its an extremist group.