Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipc_shm_arena_lend: Sync coding style in src/ipc/session/standalone/shm/arena_lend/** and src/ipc/shm/arena_lend/** #47

Open
ygoldfeld opened this issue Mar 12, 2024 · 0 comments
Labels
style-refactor An enhancement mainly for style and/or best practices; maybe minor perf benefits

Comments

@ygoldfeld
Copy link
Contributor

ygoldfeld commented Mar 12, 2024

There is a quite consistent and even codified (in flow/.../doc-coding_style.cpp) style throughout Flow-IPC (and Flow). There is one significant subset of the code where this is not completely the case: src/ipc/session/standalone/shm/arena_lend/** and src/ipc/shm/arena_lend/**.

[There are some historical reasons for this; basically this is the part written by 1 of the 2 lead developers of the original code (~at the time this became open-source). (The other guy did write shm_pool_offset_ptr_data.?pp in this area... so the style there is consistent.)]

Even there it is largely the case, but some things I can see are somewhat different; some naming, some formatting (Doxygen function doc header formatting especially), log message punctuation and grammar.

It should be pretty easy to see which parts these are and manually fix 'em up. Formally one could check against doc-coding_style.cpp too, but you get the idea.

One specific aspect is not 100% stylistic, though close: There is some (not a lot of) explicit inlining. The official style (this can be controversial with some but hopefully not too much) says do not inline explicitly; let the compiler+linker deal with it. (We build with LTO enabled, so the inlining should occur even across translation units; unless one disables it with a dedicated CMake setting.) Probably, if only for consistency, this should be fixed-up. I mention it specifically, since it can be seen as affecting the generated code and even performance; but we make the argument that, well, not really.

Related issues that should be considered at the same time (the order in which they're to be done = exercise left to reader): Flow-IPC/ipc#87, Flow-IPC/flow#74.

@ygoldfeld ygoldfeld added the style-refactor An enhancement mainly for style and/or best practices; maybe minor perf benefits label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style-refactor An enhancement mainly for style and/or best practices; maybe minor perf benefits
Projects
None yet
Development

No branches or pull requests

1 participant