Modernisation: Discussions

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Modernisation: Discussions

Post by FreddyFreddy »

I have started a process to submit PRs to continue and maybe accelerate the ongoing modernisation of FC codebase (much of which is up to 20+ years old). I am no expert in C++ but am tackling the simpler/more obvious things thrown up by my IDE. It is proving a great teacher and I am getting some good bumps by devs (appreciated).

Some issues I am unsure how to proceed, and PRs are obviously not the place for research, so I propose to do that here. If your view is that there is no point modernising, or that warnings should be ignored, then please do not come here!

To get the ball rolling:

Code: Select all

    virtual void getPoints(std::vector<Base::Vector3d> &Points,
        std::vector<Base::Vector3d> &Normals,
        float Accuracy, uint16_t flags=0) const;
Clang-Tidy: Default arguments on virtual or override methods are prohibited
There are quite a few of these.

And:

Code: Select all

public:
    ~Segment() override= default;
    virtual std::string getName() const=0;
};
Clang-Tidy: Function 'getName' should be marked [[nodiscard]]
And:

Code: Select all

private:
    /// Private new operator to prevent heap allocation.
    void* operator new (std::size_t) = delete;
Clang-Tidy: Deleted member function should be public
User avatar
chennes
Veteran
Posts: 3884
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Modernisation: Discussions

Post by chennes »

Clang-Tidy: Default arguments on virtual or override methods are prohibited
The danger with this usage is that if a later override attempts to change the default parameter, it won't work. So IMO this is good advice.
Clang-Tidy: Function 'getName' should be marked [[nodiscard]]
This is pretty nitpicky, bordering on wrong. I don't think we should be littering the codebase with [[nodiscard]] in instances where discarding the value doesn't result in either a resource leak or obviously deeply flawed calling code. Not storing the name for whatever reason doesn't strike me as quite egregious enough to warrant it, but I could be swayed on that point.
Clang-Tidy: Deleted member function should be public
I agree with this advice, this sort of code doesn't make much sense. Either delete it, or make it private: no reason to do both if the object is simply to prevent code that tries to use the function from compiling.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Modernisation: Discussions

Post by FreddyFreddy »

One that won't be popular with those not convinced about functional programming:

Code: Select all

    for (const auto & It : d->objectArray)
        if (It->isTouched() || It->mustExecute()==1)
            return true;
    return false;
Clang-Tidy: Replace loop by 'std::any_of()'
which would look like:

Code: Select all

    return std::any_of(
        d->objectArray.begin(),
        d->objectArray.end(),
        [](const auto &It) {
            return It->isTouched() || It->mustExecute() == 1
        });
Having to type the begin and end bits feels like a backward step when 99% of cases will be first to last.
Is there a performance penalty?
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Modernisation: Discussions

Post by FreddyFreddy »

Not related to Clang-tidy, but seen quite a bit

Code: Select all

            if(ext)
                linked = ext->getTrueLinkedObject(false,nullptr,0,true);
            else
                linked = o->getLinkedObject(false);
rather than the ternary

Code: Select all

            linked = ext
                ? ext->getTrueLinkedObject(false, nullptr, 0, true)
                : o->getLinkedObject(false)
which is cleaner and simpler.
User avatar
chennes
Veteran
Posts: 3884
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Modernisation: Discussions

Post by chennes »

FreddyFreddy wrote: Sat Apr 30, 2022 1:17 am Having to type the begin and end bits feels like a backward step when 99% of cases will be first to last.
Is there a performance penalty?
The most important thing here is that std::any_of more clearly expresses the intent of the code, so again I think this is solid advice for new code. I personally wouldn't troll through the code and replace the old code that uses the older idiom, but if someone else is doing the work I'm happy to review it :D . In a few years when we can standardize on C++20 we'll be able to use the ranges version, which is a bit cleaner. I'm quite certain there is no performance penalty, but it's pretty easy to test. You could even look at it in Godbolt, I'd guess the generated code is pretty similar.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Modernisation: Discussions

Post by FreddyFreddy »

chennes wrote: Fri Apr 29, 2022 3:41 am This is pretty nitpicky, bordering on wrong. I don't think we should be littering the codebase with [[nodiscard]] in instances where discarding the value doesn't result in either a resource leak or obviously deeply flawed calling code.
I'll leave it alone, for now. Some files would have HEAPS of these.
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Modernisation: Discussions

Post by FreddyFreddy »

Code: Select all

Clang-Tidy: Do not use static_cast to downcast from a base to a derived class
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Modernisation: Discussions

Post by wmayer »

FreddyFreddy wrote: Mon May 02, 2022 11:21 am

Code: Select all

Clang-Tidy: Do not use static_cast to downcast from a base to a derived class
This can be mainly ignored. There might be a few cases where a static_cast is not safe but in most cases this cast is used like this:

Code: Select all

obj = ...
if (obj->getTypeId().isDerivedFrom(Feature::getClassTypeId())) {
    static_cast<Feature*>(obj)->...
}
For clang-tidy it's probably impossible to know that the static_cast is perfectly valid in this context. The alternative would be using dynamic_cast but then the code has to be rewritten as:

Code: Select all

if (Feature* obj = dynamic_cast<Feature*>(...)) {
    obj->...
}
The worst would be to combine isDerivedFrom and dynamic_cast because then the more expensive type checks are done twice.

Code: Select all

obj = ...
if (obj->getTypeId().isDerivedFrom(Feature::getClassTypeId())) {
    dynamic_cast<Feature*>(obj)->...
}
Also a lot of code checkers will complain about this construct because the result of dynamic_cast isn't verified and thus could be null.

A valid alternative is to use our own cast function: freecad_dynamic_cast (see BaseClass.h)
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Modernisation: Discussions

Post by wmayer »

FreddyFreddy wrote: Mon May 02, 2022 11:21 am

Code: Select all

Clang-Tidy: Do not use static_cast to downcast from a base to a derived class
In anticipation for reinterpret_cast: clang-tidy and the cpp core guidelines also suggest not to use reinterpret_cast but there are several cases where it's the only valid cast operation. For example when you have to cast from PyTypeObject to PyObject you cannot use static_cast or dynamic_cast because there is no relation through inheritance between them. They are independent C structs where a PyTypeObject is an extended PyObject.

However in the current code base reinterpret_cast is often used to cast from a void pointer. This can be replaced with a static_cast.
FreddyFreddy
Posts: 176
Joined: Wed Mar 09, 2022 3:15 am
Location: Oz

Re: Modernisation: Discussions

Post by FreddyFreddy »

One day I'll understand all this. In mean-time it looks horrendous, but that is based on ignorance. What's a newbie to think? Bjarne Stroustrup: "Explicit type conversation, often called casting, is occasionally essential. However traditionally, it is seriously overused and a major source of bugs". [C++ Programming Language p.301].
Post Reply