X #1263
X #1263
Conversation
alejandro-colomar
force-pushed
the
x
branch
2 times, most recently
from
98d8375 to
14d0c3e
Compare
alejandro-colomar
force-pushed
the
x
branch
2 times, most recently
from
0f51a85 to
4933b19
Compare
alejandro-colomar
force-pushed
the
x
branch
from
b3d319c to
f54ffd9
Compare
alejandro-colomar
force-pushed
the
x
branch
3 times, most recently
from
93d59ca to
f8a1a6d
Compare
alejandro-colomar
force-pushed
the
x
branch
2 times, most recently
from
3fb734f to
913284e
Compare
There was a problem hiding this comment.
I'm concerned about the development path that led us here. This PR effectively reverts a significant amount of refactoring that was done just a few months ago.
The problem
Looking at the recent history, we've had two opposing architectural shifts in a very short time:
- Phase 1 (June 2024 - Jan 2025): a concerted effort was made to modularize these helper functions. They were intentionally moved into dedicated files (lib/alloc/x/, lib/string/strdup/xstrdup.c, etc.) across several commits (29f4f03, 3049bef, 423fd65).
- Phase 2 (This PR): we are now de-modularizing by removing all those newly created files and consolidating everything into a single macro.
This rapid reversal, while leading to a good end state, is problematic for a few key reasons:
- Wasted effort: the time developers and reviewers spent on the initial modularization has been nullified. That's a direct cost to the project's velocity.
- Increased code churn: it introduces noise into the codebase and git history. Future developers looking at the history of these files will see a confusing back-and-forth, making it harder to understand our design philosophy.
- Architectural instability: it signals a lack of a clear, long-term vision for the codebase's structure. This can make it difficult for contributors to write code that aligns with the project's direction, as that direction appears to change quickly.
This pattern also touches on a broader concern I've raised before about architectural consistency. What's challenging is that we sometimes embark on a promising direction, like the modularization effort, only to reverse it shortly after. When this happens, it becomes difficult to understand the long-term vision and the 'why' behind our structural changes, making it harder for all of us to build with confidence and stay aligned.
By the way, I added some comments inline but I think we should discuss the previous point before.
alejandro-colomar
force-pushed
the
x
branch
3 times, most recently
from
8dd1954 to
6f1060f
Compare
That modularization allowed me to see the pattern, and that this macro was possible. In some sense, it has paid off, I think. I regret not having seen that before, though;...
...I agree this churn could have been prevented if I had seen the possibility of writing this macro earlier. Sometimes, it's hard to see some transformations.
I still want different functions to be in different files, as a general rule. The differences in this case are
I'm still in favour of this modularization. I'd consider this a special case, rather than a direction change. It's similar to the macros for handling arrays, which are also one-liner macro definitions, and which we already define together with the function that they wrap. |
|
My concern isn't just about the churn in this specific instance, but about the broader pattern and our long-term architectural goals. As I mentioned before our goal is to maintain robust and simple code. The core issue, as I see it, is the lack of a clear, documented vision for our codebase's structure. You've convincingly argued this is a special case, but the challenge for me is knowing what defines a special case. Without shared principles, it's difficult to review PRs consistently. It leads to situations like this, where well-intentioned efforts seem to contradict each other, and it's hard for anyone on the team to know if they're building in the "right" direction. This situation highlights the challenge of staying aligned on architectural direction when the guiding principles aren't explicitly defined. A shared understanding of the long-term vision would help ensure we are all building towards the same robust and simple codebase we're aiming for. |
My principles for direction of the project would be:
Reducing lines of code isn't good per se; for example, we could remove blank lines, and nothing would be improved. However, if those removals come from using functions to hide code, then the code becomes smaller, and thus more readable. Those APIs need to be well designed to make sense, of course, but I don't think I can write a recipe for guiding API design, other than "try to be consistent". Some good advise to design APIs is to minimize invention. Try to find existing patterns, and wrap them. Do not try to over-engineer. Consider that those existing patterns enclose old wisdom quite often, so try to understand them and keep that wisdom in the wrapper API. |
alejandro-colomar
force-pushed
the
x
branch
2 times, most recently
from
0e6904b to
a333a6b
Compare
|
Overall I agree with Iker, and I had the same thought, "isn't all of this I do however like the idea of not having separate xZZZ implementations. Some things I look for when deciding whether something is worth the churn: If it removes duplicate code, +1. If it adds testing, without adding overdue burden (e.g. fragile testing If it makes the code easier to read. Makes it easier to say "yeah, that's If it abstracts away tedious cases or code, which this does. If it enables the compiler to catch easy to make mistakes. This is where |
I would like to add that I prioritize testing comprehensiveness - both unit and system-level coverage. Strong test coverage gives us confidence in releases and helps maintain the reliability that system administrators and users depend on. In addition, for significant architectural changes, I also look for clear justification for any reversals of recent decisions, explicit trade-off analysis, and confidence that this establishes sustainable patterns rather than one-off solutions. This helps ensure we're building toward a coherent long-term architecture rather than making reactive changes.
Yes, but I have one last change I'd like to introduce. @alejandro-colomar one final note before approving, do you mind changing the |
Yup, the renaming sounds reasonable, given the updated name of the macro. I'll do in a moment. |
Writing an x*() variant function of several functions is unnecessary. It's simpler to write a generic exit_if_null() macro that can be chained with any other calls. With such a macro, the x*() variants can be implemented as one-liner macros that are much easier to read: For example: #define xmalloc(size) exit_if_null(malloc(size)) If an error is detected, log an error, and exit(13). About why 13, I don't really know. It's just what was used previously in xmalloc(). Signed-off-by: Alejandro Colomar <[email protected]>
…_if_null() Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…_if_null() Signed-off-by: Alejandro Colomar <[email protected]>
This is much simpler. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Now that all of these are one-liners, they don't need a separate header file. Compact stuff. Signed-off-by: Alejandro Colomar <[email protected]>
3 participants
Simplify and centralize the implementation of function variants that exit on error.
Revisions:
v1b
v2
v2b
v3
#include "config.h"with quotes.v3b
v3c
v3d
v3e
v3f
v4
v4b
v5
s/X/x/. X() is a widely used name for a different thing (https://www.geeksforgeeks.org/c/x-macros-in-c/). Let's use a different name.v5b
v5c
v6
v6b
v6c
v7