pret / pokeheartgold

Decompilation of Pokemon HeartGold/SoulSilver
306 stars 97 forks source link

Decide consensus on styling for recursive/"references each other" structs AND styling for unions. #212

Open luckytyphlosion opened 1 year ago

luckytyphlosion commented 1 year ago

5. There is no consensus for defining recursive structs.

When defining a recursive struct, the struct must be declared and defined. Below are the options for declaring the struct.

Below are the options for defining the struct. Option A1 is only compatible with Option B1. Keywords/identifiers in <> are optional.

6. There is no consensus on whether unions should follow the same rule as structs.

typedef struct TextPrinterTemplate { // mention union here union StrbufForPrint currentChar; Window *window;

// rest of struct omitted

} TextPrinterTemplate;

- **Option 2:**
```c
// union typedef
typedef union StrbufForPrint {
    String *wrapped;
    const u16 *raw;
} StrbufForPrint;

typedef struct TextPrinterTemplate {
    // no mentioning union
    StrbufForPrint currentChar;
    Window *window;

    // rest of struct omitted
} TextPrinterTemplate;
red031000 commented 1 year ago

sometimes it is necessary to have the typedef and struct definition separately, usually when the structs contain pointers to eachother, in that case the de facto standard is the following

Struct1.h

#include "Struct2.h"

struct Struct1 {
    Struct2 struct2;
};

Struct2.h

typedef struct Struct1 Struct1;

typedef struct Struct2 {
    Struct1 struct1;
} Struct2;

ofc what struct contains the typedef is somewhat arbritrary, but it's usually based on which one is "higher level" or "more specific"

red031000 commented 1 year ago

as for unions, imo they should follow the same rules as structs

luckytyphlosion commented 1 year ago

sometimes it is necessary to have the typedef and struct definition separately, usually when the structs contain pointers to eachother, in that case the de facto standard is the following

Struct1.h

#include "Struct2.h"

struct Struct1 {
    Struct2 struct2;
};

Struct2.h

typedef struct Struct1 Struct1;

typedef struct Struct2 {
    Struct1 struct1;
} Struct2;

ofc what struct contains the typedef is somewhat arbritrary, but it's usually based on which one is "higher level" or "more specific"

I already mentioned this in point 5

luckytyphlosion commented 1 year ago

as for unions, imo they should follow the same rules as structs

I think the issue with unions is that knowing that it's a union is very useful context, because the way that you operate on unions is much differently than structs. One possibility is that they follow the same rules as structs, but have to be suffixed with Union.

red031000 commented 1 year ago

generally I don't think you should really be passing unions around by themselves, all unions should be within a struct, even if it is the only thing in the struct imo

luckytyphlosion commented 1 year ago

Is that really possible to enforce while matching?

red031000 commented 1 year ago

I think so? not sure metrowerks makes that much of a distinction, if it's not actually possible then imo it should be explicit in the type name

luckytyphlosion commented 1 year ago

GymmickUnion is at least one example of passing around a union itself. So it does exist in the code.

luckytyphlosion commented 1 year ago

I have realized that labelling all unions with a Union suffix may not be the right choice. Consider this union for example.

typedef union Quaternion_AsMtxF44 {
    struct {
        f32 _00, _01, _02, _03;
        f32 _10, _11, _12, _13;
        f32 _20, _21, _22, _23;
        f32 _30, _31, _32, _33;
    };
    f32 m[4][4];
    f32 a[16];
} Quaternion_AsMtxF44;

This is literally just an MtxFx44, except it uses floats instead of fixed point numbers. The extra fields are "overloads" of accessing the data in different interpretations, since one may want to view the matrix via its individual points, as a 4x4 array, or as a raw list of points. But it is not like a union where it has entirely different data depending on the context. Therefore, I propose that "overload" unions can stay as unions and do not need to be suffixed with Union, while "context" unions must be suffixed with Union.

github-actions[bot] commented 3 weeks ago

This issue has had no activity for 60 days and will be marked stale. If there is no further activity, it will be closed in 30 days.