Re: [Flightgear-devel] RFC: Subsystem redesign - part 1.

[Edward d'A. <tru...@gm...>]

On Mon, 1 Jul 2019 at 07:45, James Turner <ja...@fl...> wrote:
> On 26 Jun 2019, at 08:25, Edward d'Auvergne <tru...@gm...> wrote:
>> Cheers!  This first part should be pretty straight forward and
>> non-disruptive.  The whitespace standardisation is now restricted to
>> solely the subsystem declarations and, as you'll see, that makes
>> reviewing repetitive changes to all our 137 subsystems and 11
>> subsystem groups much, much easier.
>
> Basically looks fine, feel free to commit and then address these points: (since I'm away for two weeks now, dsidn’t want to block you)
>
> - why is it a problem to have subsystems groups have a name? Not that we need it, I just don’t get why it’s incompatible with the new class-instance-id scheme. And for Autopilot groups it was slightly useful to have thiis

It's not a problem but, with this new design, the name is handled
differently.  For example for the autopilot group it now looks like:

    // Subsystem identification.
    static const char* staticSubsystemClassId() { return "autopilot"; }

Hence the SGSubsystemGroup const char* name ctor argument Richard
introduced is no longer needed and is replaced by
staticSubsystemClassId().  I can see in my simgear branch that my
refactoring broke a little and some of the "SGSubsystemGroup: Removal
of the subsystem group naming." changes have accidentally been shifted
into the previous commit.  I'll fix that so that the SGSubsystemGroup
naming changes are all in that commit.


> - I understand the rational for the whitespace and API consistency changes, but be aware you will obscure the revision history, and make blame much less useful each time, so yes, it’s okay, but, don’t do another such change without discussing it :)

Of course.  I think in this case, where 137+11 subsystems+groups need
to be managed simultaneously, that the limited header whitespace
standardisation is worth the blame ;)


> (Or we need to define a git-hook with a whitespace linter, and I’m not sure that would be well received…..)

Hmmm, maybe a simple one blocking tab characters?  I don't think it is
worth the effort.


> - can you document the new dependency types in the enum (DOxygen comment), since I already don’t understand what they all do :)

Something like this?

"""
    /**
     * @brief Subsystem dependency structure.
     */
    struct Dependency {
        enum Type {
            HARD,               ///< The subsystem cannot run without
this subsystem dependency.
            SOFT,               ///< The subsystem uses this subsystem
dependency, but can run without it.
            SEQUENCE,           ///< Used for ordering subsystem initialisation.
            NONSUBSYSTEM_HARD,  ///< The subsystem cannot run without
this non-subsystem dependency.
            NONSUBSYSTEM_SOFT,  ///< The subsystem uses this
non-subsystem dependency, but can run without it.
            PROPERTY            ///< The subsystem requires this
property to exist to run.
        };

        std::string name;
        Type type;
    };
"""

Cheers,

Edward