Skip to content
  • Categories
  • Recent
  • Tags
  • Popular
  • Users
  • Groups
  • Search
  • Get Qt Extensions
  • Unsolved
Collapse
Brand Logo
  1. Home
  2. General talk
  3. The Lounge
  4. Recurring C++ and Qt anti-patterns
Forum Updated to NodeBB v4.3 + New Features

Recurring C++ and Qt anti-patterns

Scheduled Pinned Locked Moved The Lounge
126 Posts 17 Posters 69.1k Views 10 Watching
  • Oldest to Newest
  • Newest to Oldest
  • Most Votes
Reply
  • Reply as topic
Log in to reply
This topic has been deleted. Only users with topic management privileges can see it.
  • SGaistS Offline
    SGaistS Offline
    SGaist
    Lifetime Qt Champion
    wrote on last edited by
    #107

    AFAIK, there's nothing wrong with that. It's just that in the case you are showing, the static method has a specific behaviour that makes it unsuitable to be called like that.

    Interested in AI ? www.idiap.ch
    Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

    1 Reply Last reply
    2
    • JonBJ JonB

      From https://forum.qt.io/topic/113223/check-whether-a-script-exists-by-script-name/14

      QProcess process;
      process.setStandardOutputFile(QProcess::nullDevice());
      if (!process.startDetached(progName, args))
      ...
      

      Would anyone care to comment on why C++ allows calling a static method off an instance without (seemingly) offering the option of a warning message for it? :) (C# doesn't let me write this.)

      kshegunovK Offline
      kshegunovK Offline
      kshegunov
      Moderators
      wrote on last edited by kshegunov
      #108

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

      Would anyone care to comment on why C++ allows calling a static method off an instance without (seemingly) offering the option of a warning message for it? :) (C# doesn't let me write this.)

      Because the class is known and that's all that matters. Whether you call it through an object or with its qualified name makes no difference. Actually, there's one widespread use of that in the Qt documentation:

      int main(int argc, char *argv[])
      {
          QApplication app(argc, argv);
          return app.exec(); // QCoreApplication::exec is static
      }
      

      Read and abide by the Qt Code of Conduct

      JonBJ 1 Reply Last reply
      2
      • kshegunovK kshegunov

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

        Would anyone care to comment on why C++ allows calling a static method off an instance without (seemingly) offering the option of a warning message for it? :) (C# doesn't let me write this.)

        Because the class is known and that's all that matters. Whether you call it through an object or with its qualified name makes no difference. Actually, there's one widespread use of that in the Qt documentation:

        int main(int argc, char *argv[])
        {
            QApplication app(argc, argv);
            return app.exec(); // QCoreApplication::exec is static
        }
        
        JonBJ Offline
        JonBJ Offline
        JonB
        wrote on last edited by JonB
        #109

        @kshegunov
        But that is not my point/question. Which is: this piece of code is not the first (or the last) where someone has mistakenly written this. If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option? There is reason that e.g. C# does not allow it.

        kshegunovK 1 Reply Last reply
        0
        • fcarneyF Offline
          fcarneyF Offline
          fcarney
          wrote on last edited by
          #110

          "Within C++, there is a much smaller and cleaner language struggling to get out."
          ...
          "And no, that smaller and cleaner language is not Java or C#."
          Bjarne Stroustrup

          C++ is a perfectly valid school of magic.

          1 Reply Last reply
          0
          • JonBJ JonB

            @kshegunov
            But that is not my point/question. Which is: this piece of code is not the first (or the last) where someone has mistakenly written this. If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option? There is reason that e.g. C# does not allow it.

            kshegunovK Offline
            kshegunovK Offline
            kshegunov
            Moderators
            wrote on last edited by
            #111

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

            If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option?

            If this were a potential error, probably. Since this is almost always safe there's no reason to offer a warning.

            There is reason that e.g. C# does not allow it.

            Which is what exactly?

            Read and abide by the Qt Code of Conduct

            jsulmJ 1 Reply Last reply
            0
            • kshegunovK kshegunov

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

              If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option?

              If this were a potential error, probably. Since this is almost always safe there's no reason to offer a warning.

              There is reason that e.g. C# does not allow it.

              Which is what exactly?

              jsulmJ Offline
              jsulmJ Offline
              jsulm
              Lifetime Qt Champion
              wrote on last edited by
              #112

              @kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums). The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

              https://forum.qt.io/topic/113070/qt-code-of-conduct

              J.HilkJ kshegunovK 2 Replies Last reply
              1
              • jsulmJ jsulm

                @kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums). The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

                J.HilkJ Offline
                J.HilkJ Offline
                J.Hilk
                Moderators
                wrote on last edited by
                #113

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

                but I doubt people would care enough about those

                They don't I have taken over projects that hat on first compile 20k + warnings...

                "Every time you compile with warnings, a fairy dies! So don't forget to clap your hands during compile time. Once for each fairy!"


                Be aware of the Qt Code of Conduct, when posting : https://forum.qt.io/topic/113070/qt-code-of-conduct


                Q: What's that?
                A: It's blue light.
                Q: What does it do?
                A: It turns blue.

                1 Reply Last reply
                0
                • jsulmJ jsulm

                  @kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums). The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

                  kshegunovK Offline
                  kshegunovK Offline
                  kshegunov
                  Moderators
                  wrote on last edited by kshegunov
                  #114

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

                  @kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums).

                  Yes, I acknowledged that, but it's not an error, nor does it warrant a warning in my mind. One just have to know what they're doing/expecting of said method, which is good approach in every case. ;)

                  The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

                  That's why I compile with warnings-are-errors before I even consider deploying. The other option is just abysmal ... and can be dangerous depending on which field you're working in. So to everyone out there that ignores warnings: fix your freaking code!

                  Read and abide by the Qt Code of Conduct

                  JonBJ 1 Reply Last reply
                  0
                  • kshegunovK kshegunov

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

                    @kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums).

                    Yes, I acknowledged that, but it's not an error, nor does it warrant a warning in my mind. One just have to know what they're doing/expecting of said method, which is good approach in every case. ;)

                    The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

                    That's why I compile with warnings-are-errors before I even consider deploying. The other option is just abysmal ... and can be dangerous depending on which field you're working in. So to everyone out there that ignores warnings: fix your freaking code!

                    JonBJ Offline
                    JonBJ Offline
                    JonB
                    wrote on last edited by
                    #115

                    @kshegunov
                    I'll try to keep my remarks brief, as I don't want to dominate this thread.

                    As @jsulm said, my point is that being allowed to call a static method on an instance is not wrong or an error, but it may indicate a programmer mistake. I observe this empirically from the number of cases I have seen, such as the one I quoted from this forum.

                    If I write a statement like word;, then gcc gives me a -Wunused-value warning. If I write if (word = value) I get a -Wparentheses warning. Neither of these is "wrong", the second one in particular is perfectly useful, yet someone recognised they may indicate commonly made faux-pas. Which the user may ignore, or suppress, at their peril. Personally, I would have liked to have seen a -Wstatic-call-on-instance :)

                    kshegunovK 1 Reply Last reply
                    0
                    • JonBJ JonB

                      @kshegunov
                      I'll try to keep my remarks brief, as I don't want to dominate this thread.

                      As @jsulm said, my point is that being allowed to call a static method on an instance is not wrong or an error, but it may indicate a programmer mistake. I observe this empirically from the number of cases I have seen, such as the one I quoted from this forum.

                      If I write a statement like word;, then gcc gives me a -Wunused-value warning. If I write if (word = value) I get a -Wparentheses warning. Neither of these is "wrong", the second one in particular is perfectly useful, yet someone recognised they may indicate commonly made faux-pas. Which the user may ignore, or suppress, at their peril. Personally, I would have liked to have seen a -Wstatic-call-on-instance :)

                      kshegunovK Offline
                      kshegunovK Offline
                      kshegunov
                      Moderators
                      wrote on last edited by
                      #116

                      Yes, yes, I get that. Then you better start getting used to writing code like this:

                      int main(int argc, char *argv[])
                      {
                          QApplication app(argc, argv);
                          (void) app; // or Q_UNUSED(app);
                      
                          return QApplication::exec();
                      }
                      

                      Because an unused variable is actually a warning ... and yes in the general case the compiler can't strip it directly, because constructors and/or destructors may have side effects (if they're out-of-line, which they often are). So on the off chance of that, calling a static method on an object is much more benign is my argument. It's a logical error that you'd be able to debug quite easily, and not propagate it into the program runtime. I get people can and will fall for it from time to time, but again, it's rather benign. I'd rather see more strict warnings for implicit conversions than for this ...

                      Read and abide by the Qt Code of Conduct

                      JonBJ 1 Reply Last reply
                      0
                      • kshegunovK kshegunov

                        Yes, yes, I get that. Then you better start getting used to writing code like this:

                        int main(int argc, char *argv[])
                        {
                            QApplication app(argc, argv);
                            (void) app; // or Q_UNUSED(app);
                        
                            return QApplication::exec();
                        }
                        

                        Because an unused variable is actually a warning ... and yes in the general case the compiler can't strip it directly, because constructors and/or destructors may have side effects (if they're out-of-line, which they often are). So on the off chance of that, calling a static method on an object is much more benign is my argument. It's a logical error that you'd be able to debug quite easily, and not propagate it into the program runtime. I get people can and will fall for it from time to time, but again, it's rather benign. I'd rather see more strict warnings for implicit conversions than for this ...

                        JonBJ Offline
                        JonBJ Offline
                        JonB
                        wrote on last edited by JonB
                        #117

                        @kshegunov
                        I wish I hadn't picked the "unused variable" warning example :) Please look at the "parentheses" warning instead to compare against.

                        calling a static method on an object is much more benign [...] that you'd be able to debug quite easily

                        Then we shouldn't see too many questions about this from ppl :)

                        P.S.
                        I have always written:

                        QApplication app(argc, argv);
                        return app.exec();
                        

                        I had never even noticed QApplication::exec() is static, I presumed it was instance. Although it does no harm, I dislike this even more now...!

                        1 Reply Last reply
                        1
                        • kshegunovK Offline
                          kshegunovK Offline
                          kshegunov
                          Moderators
                          wrote on last edited by kshegunov
                          #118

                          Here's one a bit more convoluted, by yours truly:

                          GraphDialog::GraphDialog(QWidget * parent)
                              : QDialog(parent), chartsModel(&graphMeta)
                          {
                              // ...
                              QObject::connect(ui.chartsView->selectionModel(), &QItemSelectionModel::currentChanged, [this] (const QModelIndex & current) -> void  {
                                  ui.chartEdit->setChart(current.isValid() ? &graphMeta.charts[current.row()] : nullptr);
                              });
                              QObject::connect(ui.createChartButton, &QPushButton::clicked, &chartsModel, &RbMeta::ChartsModel::addChart);
                              // ...
                          }
                          

                          where chartsModel is editable (basically an adapter on top of graphMeta) and my types are POD, graphMeta.charts is QVector .

                          I'm curious whether someone spots it (and no it's not immediately evident, I still haven't hit it, but it's there).

                          Read and abide by the Qt Code of Conduct

                          kshegunovK 1 Reply Last reply
                          0
                          • kshegunovK kshegunov

                            Here's one a bit more convoluted, by yours truly:

                            GraphDialog::GraphDialog(QWidget * parent)
                                : QDialog(parent), chartsModel(&graphMeta)
                            {
                                // ...
                                QObject::connect(ui.chartsView->selectionModel(), &QItemSelectionModel::currentChanged, [this] (const QModelIndex & current) -> void  {
                                    ui.chartEdit->setChart(current.isValid() ? &graphMeta.charts[current.row()] : nullptr);
                                });
                                QObject::connect(ui.createChartButton, &QPushButton::clicked, &chartsModel, &RbMeta::ChartsModel::addChart);
                                // ...
                            }
                            

                            where chartsModel is editable (basically an adapter on top of graphMeta) and my types are POD, graphMeta.charts is QVector .

                            I'm curious whether someone spots it (and no it's not immediately evident, I still haven't hit it, but it's there).

                            kshegunovK Offline
                            kshegunovK Offline
                            kshegunov
                            Moderators
                            wrote on last edited by
                            #119

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

                            I'm curious whether someone spots it (and no it's not immediately evident, I still haven't hit it, but it's there).

                            Well apparently not. I'll tell for funsies:

                            &graphMeta.charts[current.row()]
                            

                            Passing a vector element by address is a recipe for disaster when the vector regrows in RbMeta::ChartsModel::addChart (where chartsModel(&graphMeta) is relevant here, again passing by reference in the initializer).

                            Read and abide by the Qt Code of Conduct

                            1 Reply Last reply
                            3
                            • kshegunovK Offline
                              kshegunovK Offline
                              kshegunov
                              Moderators
                              wrote on last edited by kshegunov
                              #120

                              Hello, It's me.
                              I've been saying it for ages, but well here it goes again: C++'s notorious for a good reason ... it's never too late to shoot yourself in the foot. Here's one nasty piece of code:

                              Say you delegate the constructor of some QObjects. A well-intentioned endeavour:

                              RbGraphWidget::RbGraphWidget(const RbGraphId & id, QWidget * parent)
                                  : RbGraphWidget(new RbGraph(this), parent)
                              {
                              

                              The above gloriously segfaults, though. Here's a partial trace:

                              1   QObject::thread                qobject.h                 132  0x7ffff6d9f538 
                              2   QObject::QObject               qobject.cpp               915  0x7ffff6daacbd 
                              3   RbGraph::RbGraph               rbgraph.cpp               76   0x55555559384c 
                              4   RbGraphWidget::RbGraphWidget   rbgraphwidget.cpp         76   0x55555559a3a6
                              ...
                              

                              So the moral of the story is: the road to hell is paved with good intentions. Anyway, the problem is that QObject requires the parent to have been initialized at the point of the constructor call. The following is correct (reparenting can be done in the delegate):

                              RbGraphWidget::RbGraphWidget(const RbGraphId & id, QWidget * parent)
                                  : RbGraphWidget(new RbGraph(), parent)
                              {
                              

                              In the usual use case the problem isn't present:

                              RbGraphWidget::RbGraphWidget(QWidget * parent)
                                  : QWidget(parent), source(new RbGraph(this)) //< `this` is fully initialized so we have no problem
                              {
                              

                              Read and abide by the Qt Code of Conduct

                              1 Reply Last reply
                              1
                              • Kent-DorfmanK Offline
                                Kent-DorfmanK Offline
                                Kent-Dorfman
                                wrote on last edited by Kent-Dorfman
                                #121

                                yeah...referencing "this" in the delegated call is bad juju...I still prefer tools that "allow" me to shoot myself in the foot thought.

                                Here's one that gets people in trouble when they start parsing network buffers.

                                uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
                                int i = *(int*)&buffer[2];
                                // bonus if you understand the potential problem...and I don't mean use of C-style casts as opposed to C++ casts
                                

                                the correct way is either a pragma pack(1) union of the buffer array and a struct containing the interesting elements, or the following:

                                uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
                                int i=0;
                                memcpy(&i,  &buffer[2], sizeof(int));
                                
                                kshegunovK 1 Reply Last reply
                                0
                                • Kent-DorfmanK Kent-Dorfman

                                  yeah...referencing "this" in the delegated call is bad juju...I still prefer tools that "allow" me to shoot myself in the foot thought.

                                  Here's one that gets people in trouble when they start parsing network buffers.

                                  uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
                                  int i = *(int*)&buffer[2];
                                  // bonus if you understand the potential problem...and I don't mean use of C-style casts as opposed to C++ casts
                                  

                                  the correct way is either a pragma pack(1) union of the buffer array and a struct containing the interesting elements, or the following:

                                  uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
                                  int i=0;
                                  memcpy(&i,  &buffer[2], sizeof(int));
                                  
                                  kshegunovK Offline
                                  kshegunovK Offline
                                  kshegunov
                                  Moderators
                                  wrote on last edited by
                                  #122

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

                                  the correct way is either a pragma pack(1) union of the buffer array and a struct containing the interesting elements

                                  That has a plethora of problems itself. The standard dictates that reading one field and writing another in a union is undefined behaviour. (although compilers implement it correctly).

                                  or the following:

                                  That's the proper way to do it, in my opinion.

                                  Read and abide by the Qt Code of Conduct

                                  1 Reply Last reply
                                  0
                                  • Kent-DorfmanK Offline
                                    Kent-DorfmanK Offline
                                    Kent-Dorfman
                                    wrote on last edited by
                                    #123

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

                                    That has a plethora of problems itself. The standard dictates that reading one field and writing another in a union is undefined behaviour. (although compilers implement it correctly).

                                    Actually, that's kind of news to me. I'd love to see the spec section that defines it as undefined, as that seems to negate the purpose of unions...so I'd hope that compilers implement it correctly. C? C++? or both?

                                    kshegunovK 1 Reply Last reply
                                    0
                                    • Kent-DorfmanK Kent-Dorfman

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

                                      That has a plethora of problems itself. The standard dictates that reading one field and writing another in a union is undefined behaviour. (although compilers implement it correctly).

                                      Actually, that's kind of news to me. I'd love to see the spec section that defines it as undefined, as that seems to negate the purpose of unions...so I'd hope that compilers implement it correctly. C? C++? or both?

                                      kshegunovK Offline
                                      kshegunovK Offline
                                      kshegunov
                                      Moderators
                                      wrote on last edited by
                                      #124

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

                                      so I'd hope that compilers implement it correctly. C? C++?

                                      No I was talking about the C++ standard. C99 and C11 are both adamant that it is well defined and does what you'd expect (which thankfully every cpp compiler I've worked with also does, but the cpp standard doesn't make such a provision).

                                      Actually, that's kind of news to me. I'd love to see the spec section that defines it as undefined, as that seems to negate the purpose of unions...

                                      Here's a decent answer:
                                      https://stackoverflow.com/questions/11373203/accessing-inactive-union-member-and-undefined-behavior

                                      Read and abide by the Qt Code of Conduct

                                      1 Reply Last reply
                                      3
                                      • fcarneyF Offline
                                        fcarneyF Offline
                                        fcarney
                                        wrote on last edited by
                                        #125

                                        Repeat: "A Loader won't load a Window."

                                        C++ is a perfectly valid school of magic.

                                        1 Reply Last reply
                                        0
                                        • aha_1980A aha_1980

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

                                          Why doesn't delete set the pointer to null then? That seems like it may be an antipattern in and of itself.

                                          I have indeed asked that myself. If someone has the correct answer for that, I'm all ears.

                                          S Offline
                                          S Offline
                                          starkm42
                                          wrote on last edited by
                                          #126

                                          https://isocpp.org/wiki/faq/freestore-mgmt#delete-zero this might answer you question.

                                          1 Reply Last reply
                                          0

                                          • Login

                                          • Login or register to search.
                                          • First post
                                            Last post
                                          0
                                          • Categories
                                          • Recent
                                          • Tags
                                          • Popular
                                          • Users
                                          • Groups
                                          • Search
                                          • Get Qt Extensions
                                          • Unsolved