Recurring C++ and Qt anti-patterns



  • Given the code:
    modules.h

    #ifndef MODULES_H
    #define MODULES_H
    
    #include <string>
    
    void reg_module(int type, std::string name, int initedValue);
    
    #endif // MODULES_H
    

    modules.cpp

    #include "modules.h"
    
    using namespace std;
    
    struct Modules
    {
        Modules(): m_initedValue(0){}
        int m_type;
        string m_name;
        int m_initedValue;
    } global_modules_struct[128];
    
    void reg_module(int type, std::string name, int initedValue){
        global_modules_struct[type].m_type = type;
        global_modules_struct[type].m_name = name;
        global_modules_struct[type].m_initedValue = initedValue;
    }
    

    moduletype.h

    #ifndef MODULETYPE_H
    #define MODULETYPE_H
    
    // nothing here
    
    #endif // MODULETYPE_H
    

    moduletype.cpp

    #include "moduletype.h"
    #include "modules.h"
    
    struct SomeModule{
        SomeModule(){
            reg_module(10, "some type", 5);
        }
    } SomeModuleInstance;
    

    Ignore obvious indexing bounds checking issues for the array itself. Also ignore external array indexing possibly being out of bounds.

    I just ran into a form of this problem in our code and it did not exhibit issues in Linux (that we know of) and did show issues in Windows. Linux used gcc and Windows used mingw. Same version of Qt 5.12.2 etc. Once identified it was really easy to see why this is a big issue.

    Edit:
    Technically global_modules_struct is not really global either. So ignore the misleading name.


  • Moderators

    I had to dig through this thing once, only the real code was like a hundred times longer and more convoluted.

    // Library.h statically linked to and included in DLL and EXE
    struct SomeType
    {
       int typeId();
    };
    Q_DECLARE_METATYPE(SomeType);
    
    // Library.cpp
    int SomeType::typeId()
    {
        return qMetaTypeId<SomeType>();
    }
    
    // main app
    SomeType& var1 = getItFromDLL();
    SomeType& var2 = getItFromEXE();
    
    bool same = var1.typeId() == var2.typeId(); // nope
    

    Pretty ugly thing to debug, especially since once in blue moon it actually works :/


  • Banned

    This post is deleted!


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

    Yes, it's very debatable :D I did find a few occasions where it was useful (latest example: modifying behaviour of QTreeView without patching Qt - I have emitted a signal from const overloaded method and did my modifications there), but I agree it does not feel "right".

    Actually, I have zero problem with this. The way my mind works it makes perfect sense, as the signal is a message to a receiving class (any class). It's not the sender method that modifies the object state. It is the message. My mind differentiates between the two.



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

    Apparently the standard allows for it:
    https://stackoverflow.com/questions/704466/why-doesnt-delete-set-the-pointer-to-null
    The creator himself wonders why it isn't so. Its like C++ is this beautiful, amazing, and now, WILD animal roaming free in cyberspace... Yeah, maybe the analogy isn't all that great, but it does conjure up a cool picture.

    Jumping back a few months on this one, but I think the decision to leave alone the pointer value upon an object delete is solid. If I understand the standard properly, the target of a delete can be an lvalue or and rvalue. So delete 0x34fc3d2200 should be a valid operation, right? How ya gonna change the value of an rvalue (in a traditional sense)?


  • Moderators

    Imagine clearing some sort of array:

    for(type* ptr = some_array; something ; ++ptr)
    {
         delete ptr;
    }
    

    Now imagine delete would zero that pointer. Do you see the problem? You would have to make another, temporary, pointer just so you zero the copy and your original doesn't get changed. In other words you're paying for what you don't use or even want. There's also problem of const pointers or pointers that you got from external APIs that do their own bookkeeping and might actually need that pointer value even after delete. It would create more problems than it solves.



  •         int 🥩=1;
            int 🧀=1;
            int 🥬=1;
            int 🍞=1;
            int 🍅=1;
            int 🥪=🥩+🥬+🍅+🧀+🥩;
            cout << 🥪 << endl;
    

    Fails to compile in C++17...



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

        int 🥩=1;
        int 🧀=1;
        int 🥬=1;
        int 🍞=1;
        int 🍅=1;
        int 🥪=🥩+🥬+🍅+🧀+🥩;
        cout << 🥪 << endl;
    

    Fails to compile in C++17...

    What's this "int" stuff? Doesn't the 17 standard deduce the type based on the rvalue? Not that I think that is necessarily a good thing though.



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    🍞

    The real problem is this variable is unused.



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

    The real problem is this variable is unused.

    So in 17 unused variables are errors instead of warnings?



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    So in 17 unused variables are errors instead of warnings?

    No, its just a bug in the code for a samich.



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

    No, its just a bug in the code for a samich.

    Samich... Are you a yinzer?



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    Are you a yinzer?

    Had to look it up. Based on what I read, no. Not sure where I heard sandwich being called samich though. I am in western USA.


  • Moderators

    Being an a-hole as a recruiter:

    What does o() mean?
    What does o.o mean?
    What does o->o mean?
    What does o-->o mean?
    What does o()--<=>--o() mean? Fun fact - crashes MSVC (yes, the compiler, not the compiled program)
    What does [](){;o()++<=>++o();}() mean?


  • Qt Champions 2017

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

    I had to dig through this thing once, only the real code was like a hundred times longer and more convoluted.
    [Snip]
    Pretty ugly thing to debug, especially since once in blue moon it actually works :/

    Indeed. Although, this is windows specific. It works correctly on Linux as the symbol resolution happens at run time.


  • Moderators

    @chris-kawa wow, some of those you don't encounter every day...


  • Moderators

    Yeah, it was a bit out of topic. Here's one anti-pattern I encounter something like 9/10 code reviews:

    auto widget = new SomeWidget(some_widget);
    auto layout = new SomeLayout(some_other_widget);
    layout->addWidget(widget);
    

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

    auto widget = new SomeWidget();
    auto layout = new SomeLayout();
    layout->addWidget(widget);
    some_other_widget->setLayout(layout);
    

    If you don't see it - count how many times parents need to be changed and imagine there's not one but, say, 50 widgets and layouts.
    For extra sweetness do the same when the parent widget is visible - how many times layouts need to be recalculated?


  • Qt Champions 2018

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

    auto widget = new SomeWidget(widget);

    I hope this is a typo :-)


  • Moderators

    @jsulm Sure, sorry, fixed :)



  • @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



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

  • Qt Champions 2018

    Hi @fcarney,

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

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



  • 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);
    }
    

  • Lifetime Qt Champion

    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.



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


  • Qt Champions 2018

    @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


  • Qt Champions 2017

    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
    

  • Qt Champions 2018

    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);


  • Qt Champions 2018

    @kshegunov

    What I meant was the following example:

    8ac16113-6178-40b1-9ce0-232de08d5cb6-image.png

    which is already an improvement on poorly formatted code.

    Regards



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



  • 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. 😀


Log in to reply