LadybirdBrowser / ladybird

Truly independent web browser
https://ladybird.org
BSD 2-Clause "Simplified" License
22.38k stars 998 forks source link

Type style in code #1084

Open dawmd opened 3 months ago

dawmd commented 3 months ago

After skimming through a number of header files, I notices that although code used in a specific header seems to abide by the same formatting and style, there's no consistency across different files. It's definitely a consequence of many users contributing to the repository (which is a good thing), but at the same time, it may lead to some issues down the path the project will go.

Examples include:

I'm not knowledgeable in how much clang-format and clang-tidy can do, but I can imagine those points may be difficult to be enforced by linters like that. I don't suggest taking a specific path, but I think it would be worthwhile to add some guidelines regarding those things. If not explicitly in a document, then during code review.

ADKaster commented 3 months ago

using types that may not be present in C++, e.g. size_t instead of std::size_t,

We don't use std:: types. using types from <cstddef> instead of <stddef.h> is a non-goal

using types whose size may vary from platform to platform, e.g. int instead of AK::u32 (or some other width that's required in a context),

There is some old code that uses int in places where it shouldn't, but we're pretty consistent that new code uses named types from AK/Types.h. That should already be in the coding style document

using auto in global declarations

We use auto consistently all over the codebase, and rely on the type either not actually mattering due to the usage patterns, or being obvious from the function being called. avoiding it for global declarations seems a non sequitur.

If you have concrete suggestions for documentation updates, please open a PR. But in summary, the response to these consistency issues are "no, we aren't going to use std::size_t", "yes, new code should prefer u32 and other AK/Types.h types" and "no, we're going to use auto liberally".

dawmd commented 3 months ago

We don't use std:: types. using types from <cstddef> instead of <stddef.h> is a non-goal

I see. If that's intended, then it's absolutely understandable. Same for your response to the other points.

If you have concrete suggestions for documentation updates, please open a PR.

Sure, I'll try to post something