[s]Inheritance[/s]Incompetence tax



  • More horrors from the [s]codebase[/s]lurking swamp we are saddled with at work, thanks to $vendor:

    [code]
    class List_of_Foos: public Fakie_List_Head<Foo > { /...*/ };

    template <typename T>
    class Fakie_List_Head<T>: public Fakie_List_Interface< class list<T> >, public list<T> { /*... */ };

    template <typename Container>
    class Fakie_List_Interface {
    Container* m_Container;
    /* ... */
    };
    [/code]

    and, the classic:
    [code]
    class Vendor_String: public string { /* ... */ };
    [/code]



  • Well it's not like you can just name instances after what it's supposed to represent...



  • Necro-fun: it turns out that some versions of Fakie_List_Interface don't even compile on a modern compiler because they depend on ADL of unqualified names.

    This isn't as bad as what I just saw, though:
    [code]
    class DtTmRep
    {
    /* ... /
    void DtTmRep::someFunc(
    /
    args /)
    {
    /
    ... /
    }
    /
    ... */
    };
    [/code]

    How'd that [b]ever[/b] compile?



  • Turning this into a code snippets thread:

    [code]
    typedef (*blah_blah_blah) (const SomeType& arg, const Vendor_String& str1, SomeOtherType& arg2, const Vendor_String& str2 = "");
    [/code]



  • Necro-ing and quoting because I can no longer edit the OP to explain it:
    @tarunik said:

    [code]
    class List_of_Foos: public Fakie_List_Head<Foo > { /.../ };
    template <typename T>
    class Fakie_List_Head<T>:
    public Fakie_List_Interface< class list<T> >,
    public list<T>
    { /
    ... / };
    template <typename Container>
    class Fakie_List_Interface {
    Container
    m_Container;
    /* ... */
    };
    [/code]

    For non-C++-inclined folks, this code is a gross misunderstanding of almost everything about inheritance. First, it's a very dumb way to wrap std::list: if you want to do that in the first place, you're much better off using list as a representation of your wrapper-class, even though you will be coding forwarding functions in that case -- list was intended to be a value type to begin with, and inheriting from it exposes you to slicing hazards whenever you pass the container itself by reference or pointer. Second, a linked-list is almost never the right container in this day and age: both vector and deque have much better spatial locality, which means that the performance gains you get on insert/delete in a linked list vs. a vector are going to be balanced out by the linked list causing cache misses. This is atop a completely gratuitous case of multiple inheritance; Fakie_List_Interface basically has no reason to exist whether you are treating the list as a value type or an intrusive construction, and whatever it does could be provided directly by Fakie_List_Head, instead.

    Finally, and this is TRWTF in this code: it creates an object with both a container and a pointer to that very container in it! Holding a pointer into part of yourself is a useless waste of sizeof(void*), considering that you already have this wherever you have said sub-pointer!



  • @tarunik said:

    class DtTmRep
    {
    /* ... /
    void DtTmRep::someFunc(
    /
    args /)
    {
    /
    ... /
    }
    /
    ... */
    };

    How'd that ever compile?

    A little something called Visual Studio.

    @tarunik said:

    typedef (*blah_blah_blah) (const SomeType& arg, const Vendor_String& str1, SomeOtherType& arg2, const Vendor_String& str2 = "");

    I don't get this. It's a regular function pointer definition, no? (C++ being TRWTF aside)
    EDIT: Well aside from the fact it doesn't compile both on account of no return type and on account of default arguments not being allowed in typedefs. (Which makes sense - the only reason I didn't pick up on this sooner is that C++ allows far weirder* things)

    [size=5]* things far more horrible/awesome[/size]



  • @created_just_to_disl said:

    A little something called Visual Studio.

    *chuckles* Turns out, G++ will accept it with a warning in -fpermissive mode, so it likely was accepted by old versions of G++ with nary a peep.

    @created_just_to_disl said:

    EDIT: Well aside from the fact it doesn't compile both on account of no return type and on account of default arguments not being allowed in typedefs.

    The no return type may have been my fault for mis-anonymizing the code: the default argument in the typedef, though, is what caused the compiler to reject it.



  • @tarunik said:

    and, the classic:

    class Vendor_String: public string { /* ... */ };

    I wanted to comment about how lucky you are that the code at least has the ": public string", but given how awful the c++ stl APIs are, I'm not entirely sure if it's luck.



  • @created_just_to_disl said:

    I wanted to comment about how lucky you are that the code at least has the ": public string", but given how awful the c++ stl APIs are, I'm not entirely sure if it's luck.

    Awful as in 'doesn't fit your OO-y expectation of how containers should be?' Or awful as in 'doesn't provide everything you are expecting?' Or as in 'I have some beef with how iterators work?' Or...?

    Filed under: CamelCase fanatics need not apply



  • Yes.


  • Discourse touched me in a no-no place

    @tarunik said:

    Awful as in 'doesn't fit your OO-y expectation of how containers should be?' Or awful as in 'doesn't provide everything you are expecting?' Or as in 'I have some beef with how iterators work?' Or...?

    C++ is the main remaining vestige of “inherit from standard library classes” fun, at least as common community practice. Other language communities have figured out that this is a bit of an anti-pattern; it's just so hard to get right.

    (My beefs with C++ are in how confusing things are when they go wrong and how finicky things are with matching headers to compilers and link libraries. It's just so damn painful at times, and sometimes the only fix is to switch to a different version of the compiler. Why? I dunno…)



  • @dkf said:

    C++ is the main remaining vestige of “inherit from standard library classes” fun, at least as common community practice. Other language communities have figured out that this is a bit of an anti-pattern; it's just so hard to get right.

    Well, the modern C++ world has figured out that this is a bad practice as well; but, as is all-too-typical, the code-monkeys who wrote our system did so in complete isolation from any notion of 'best practices'.



  • Necro-ing again, because this WTF isn't quite worthy of a new thread, but:

    [code]
    somedir/MoronCode.C:128: error: declaration of C function ‘void gethostname(char*, int)’ conflicts with
    /usr/include/unistd.h:898: error: previous declaration ‘int gethostname(char*, size_t)’ here
    [/code]


  • Discourse touched me in a no-no place

    @tarunik said:

    Necro-ing again, because this WTF isn't quite worthy of a new thread, but:

    somedir/MoronCode.C:128: error: declaration of C function ‘void gethostname(char*, int)’ conflicts with
    /usr/include/unistd.h:898: error: previous declaration ‘int gethostname(char*, size_t)’ here</blockquote>
    

    Obviously you fix that by declaring MoronCode.C's version to be static. That way it doesn't conflict with the regular version used in other modules, right?



  • @FrostCat said:

    Obviously you fix that by declaring MoronCode.C's version to be static. That way it doesn't conflict with the regular version used in other modules, right?

    MoronCode.C doesn't have a definition of gethostname to begin with, which is the great irony of this all...it wanted the regular gethostname but forward declared it manually with the wrong prototype instead of simply using #include <unistd.h> like the rest of us who actually know what header files are for.


  • Discourse touched me in a no-no place

    @tarunik said:

    MoronCode.C doesn't have a definition of gethostname to begin with, which is the great irony of this all

    Yeah, that was actually obvious from the errors.

    I don't know why you'd put your own declaration of a library function in instead of just using #include.



  • @FrostCat said:

    I don't know why you'd put your own declaration of a library function in instead of just using #include.

    But #include files are big and will blow up your compile time.


  • Discourse touched me in a no-no place

    @EvanED said:

    But #include files are big and will blow up your compile time.

    *coughprecompiled headerscough*



  • Precompiled headers have their own issues (IMO they don't play well with the code you'd want in the absence of tool issues so you have a "nicer code" and "builds faster" competition that "builds faster" has a hard time winning), and I kind of doubt they entirely solve the problem anyway.

    But anyway, I was only... maybe 5% serious. That's actually a legit concern in some isolated cases (C++ stuff is the most likely to offend because templates are where the first offenders arise), but that doesn't apply in the case of unistd.h.

    My statement had just enough truth in it to be not complete BS, which produces the best kind of troll. :-)


  • ♿ (Parody)

    @tarunik said:

    Necro-ing again, because this WTF isn't quite worthy of a new thread, but:

    [code]
    somedir/MoronCode.C:128: error: declaration of C function ‘void gethostname(char*, int)’ conflicts with
    /usr/include/unistd.h:898: error: previous declaration ‘int gethostname(char*, size_t)’ here
    [/code]

    TRWTF is the capital .C, right? Goddamnit people, explain your WTFs!


  • ♿ (Parody)

    @EvanED said:

    But anyway, I was only... maybe 5% serious.

    10 or 15 years ago you'd have had a serious point, though.


Log in to reply