Some classes are forward declared as structs and vice versa

[Jussi Pakkanen]

The class definitions of Mir are unclean. There are tons of classes that are in fact structs but which are forward declared as classes.

As an example, Rectangle is actually a struct but the code base is littered with forward declarations of type "class Rectangle".

Gcc as of current will not warn about these. Clang will, which makes the code base uncompilable with Clang, meaning we can't use all the cool stuff that Clang provides (static code analysis etc). It is also probable that Gcc will start complaining about this eventually.

Since there are so many of these, the only sane way to solve it is to provide a mir_forward_decls.h, which only contains lines such as

struct MirFoo;
struct MirBar;
class MirSomething;

and have everyone include that instead of trying to guess the type of forward declarations all over the code base.

This follows the standard approach of "only ever declare anything once".

[Alan Griffiths (alan-griffiths)]

I just wonder why clang warns about this. The code is conforming and not dangerous.

[Jussi Pakkanen (jpakkane)]

The reason Clang warns about this is quite simple: there are two conflicting definitions for a type within a single compilation unit. This issue does not cause Clang to stop the build, but only print a warning. But since we compile with -Werror, the compilation dies.

Even if we did not do that, the amount of warnings spewed is large. All in all it is better to fix this sooner rather than later. (FWIW having a single file for forward declarations is also suggested by many coding style references.)

[Alan Griffiths (alan-griffiths)]

But there are not two conflicting declarations - any more than "extern int const pi" and "const int pi = 3" is conflicting.

BTW I'm not averse to changing the code - although I'd prefer to see an MP. However, I do wonder about the rationale for the warning.

[Michi Henning (michihenning)]

The specification (7.1.6.3/3) is not totally clear on the matter:

"class-key attribute-specifier-seqopt nested-name-specifieropt identifier

The class-key or enum keyword present in the elaborated-type-specifier shall agree in kind with the declaration to which the name in the elaborated-type-specifier refers. This rule also applies to the form of elaborated-type-specifier that declares a class-name or friend class since it can be construed as referring to the definition of the class. Thus, in any elaborated-type-specifier, the enum keyword shall be used to refer to an enumeration (7.2), the union class-key shall be used to refer to a union (Clause 9), and either the class or struct class-key shall be used to refer to a class (Clause 9) declared using the class or struct class-key."

The spec doesn't clearly say that "class-key" must be the same (class or struct) everywhere. My reading suggests that the intent was to permit a class to be forward-declared as a struct, but it's possibly to read the above either way. ("*either* the class or struct class-key shall be used to refer to a *class* declared using the class *or* struct class-key")

Seeing that the only difference between a struct and class is the default visibility of members, the discussion is academic in the sense that a forward-declared class or struct cannot be dereferenced, so the default visibility of the members is irrelevant in the compilation unit that uses the forward declaration. In all other respects, class and struct and are the same, so I cannot envision any program that could possibly change semantics or otherwise be ill-formed just because someone has forward-declared a class as a struct, or vice versa.

In my opinion, clang is being needlessly anal-retentive in this case.

If compilation with clang is a project goal, I'd suggest to change the code to get rid of the warning, simply because having code compile without warnings is useful. (If there are lots of warnings as a matter of course, it gets too easy to miss the one warning among the noise that I should have paid attention to.)

I'm not sure whether a header with all the forward declarations is the right way to go though. In particular, it will create coupling between many unrelated modules just so a developer doesn't have to worry about knowing whether something is a class or a struct. If one changes to the other, we end up with a compilation avalanche where tons of stuff gets recompiled that, otherwise, wouldn't need recompilation.

[Alan Griffiths (alan-griffiths)]

Michi, I don't like the idea of a single forward declaration header included everywhere either. OTOH something more granular, or just changing "struct" <=> "class" in a few places is fine by me.

[Daniel van Vugt (vanvugt)]

If this is a hurdle to clang support then we certainly need to resolve it one way or the other.

I would suggest that in the client header you keep:
    #ifndef __MIR_LIBRARY_SOURCE__
    typedef struct MirFoo MirFoo;
    #endif
and in the implementation (client library) do:
    #define __MIR_LIBRARY_SOURCE__ 1
    typedef mir::ClientFoo MirFoo;

[PS Jenkins bot (ps-jenkins)]

Fix committed into lp:mir at revision None, scheduled for release in mir, milestone 0.0.2