[vim/vim] session.c: makeopens() is somewhat harder to read due to error checks (PR #20818)
Illia Bobyr
Problem: session.c: makeopens() is somewhat harder to read due to error
checks
Solution: Use macros to reduce visual noise
put_line() and fprintf() invocations require error checks, making their usage site a bit more cluttered than necessary. We are always checking the return value and fail in case of an error.
But the error is not likely, so we can hide (and also ensure) the error checks in a macro. This makes the rest of the code easier to read, as we are now writing explicitly just the "happy path".
This change only touches "makeopens()" for simplicity. Most other functions can be refactored to use new macros as well. Some just need to introduce common error recovery similarly to how it is done in "makeopens()".
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20818
Commit Summary
- 541606b session.c: makeopens() is somewhat harder to read due to error checks
File Changes
(1 file)
- M src/session.c (222)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20818)Thanks for the careful write-up, but I am going to decline this.
The explicit "if (... == FAIL) goto fail;" is the house style across the whole C
source (viminfo.c, bufwrite.c, and so on), and session.c matches it. The macros
hide the goto, so the exit is no longer visible at the call site, and they make
this one file an island with its own conventions. For the people who maintain
this code that is harder to read, not easier.
It is also a non-functional churn with real regression risk in a complex file,
and "it reads better to me" is a bar that would apply to every file with no
natural stopping point. So I would rather keep session.c as it is.
If a specific function is genuinely hard to follow (ex_mkrc), extracting helper
functions keeps the control flow explicit while making each piece smaller.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Illia Bobyr
illia-bobyr left a comment (vim/vim#20818)Thanks for the careful write-up, but I am going to decline this.
The explicit "if (... == FAIL) goto fail;" is the house style across the whole C source (viminfo.c, bufwrite.c, and so on), and session.c matches it. The macros hide the goto, so the exit is no longer visible at the call site, and they make this one file an island with its own conventions. For the people who maintain this code that is harder to read, not easier.
Even within session.c there are 3 styles of error handling.
I listed them in the (issue)[https://github.com//issues/20819].
As all the error checking is repeated in every function because of the duplication.
I've seen:
- Return immediately on any error.
- Jump to a cleanup block on error.
- Set a flag on any error, and check this flag on all subsequent operations and at the end of the function.
So, when you are saying that the existing code is consistent and there is a convention, I would argue that it is not a very stable convention, even within one file.
Consider that macros do force uniform error handling, as long as you use them.
Do you really think this explicit error handling is a good thing?
While I can see how you can get used to it, it is not the same as it being a good practice or style.
Obviously the error checking in itself is good. What I'm asking about is the way it is expressed right now in the functions in session.c.
Perhaps you may consider changing error handling in the rest of the code base over time as well?
I do not know the Vim source as well as you do, but I've read most of it over time, and I think a lot of other functions are much easier to follow.
Perhaps it is because they rely on a lot of invariants, and thus there are fewer spots where errors can happen.
After all, the application's boundary is exactly where every operation may potentially fail.
But, I would argue, it does not justify all the "error checking noise" that arises from a style where every function call return value needs to be checked.
Even more so, as error checking is so prevalent in the code that deals with the external world, it seems reasonable to think about some design for error checking that is more than just the bare bones of "check every return value".
It is also a non-functional churn with real regression risk in a complex file, and "it reads better to me" is a bar that would apply to every file with no natural stopping point. So I would rather keep session.c as it is.
I think the way I split the changes, it is not that difficult to verify.
Each PR affects only a single function. And as such, I would argue the risk is rather low.
This code is also covered by multiple tests.
But I could spend a bit of time to try to reduce the migration risk if you are interested.
I would also think that saying "it reads better to me" is unfair.
While C is not my main language, I've read C code in multiple different projects, and I would argue that such a frequent need for error checking in session.c does make it objectively harder to read.
I also believe that my argument that there is a big difference between the happy path and the error handling path is very objective.
Many, if not most, programming languages provide tools to distinguish these two control flow paths specifically because mixing them creates code that is harder to read and to maintain.
There is obviously a lot of subjectivity in how to handle errors, how to express error handling, and such.
But claiming that this is just me is not objective.
If a specific function is genuinely hard to follow (ex_mkrc), extracting helper functions keeps the control flow explicit while making each piece smaller.
ex_mkrc is not the only one that is hard to follow, but it is especially hard :)
I did change a few of the helpers to make this function and some other code a little easier.
For example, ses_put_fname() is always followed by a new line, and is always preceded by fputs(). And similarly ses_buf_fname() is often preceded by fputs().
3 commits that adjust ses_buf_fname() and ses_buf_fname() do not update the call sites right away, as most of the improvements come in the clean-up that is done in each individual function in subsequent commits when they are converted to use macros.
Each macro produces a single line. Which, I think, also makes it easier to follow the updated version. It matches closer with what the produced session file looks like.
One statement is one output line, except for complex cases.
I apologize for going into the details here, but I have two goals:
- Show that I did think about how to improve the readability of this file. Not just though "I like macros; let's use them."
- I would like to understand which changes you might be willing to accept.
While I understand where you are coming from when you say "non-functional churn," I think it is a somewhat shallow assessment.
Most functional changes are focused and local. They do not necessarily produce good style when combined, and in most cases, they produce inconsistent design.
You need to actively do maintenance changes that clean up the code base based on the aggregated result you have.
This is technical dept.
I would argue that the changes I'm proposing are targeting technical debt accumulated in session.c.
Both regarding error handling and also in readability in general.
Technical debt cleanup is nonfunctional, so this part is correct.
But calling it "churn" is a bad idea, as it makes it look like it is something bad.
While, in reality, it is the technical debt accumulation that is a bad thing.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20818)Thanks, this is a thoughtful reply. You are right that session.c mixes three
error-handling styles and that separating the happy path from the error path is
a real principle. I still have to decline, though, for two reasons.
Scope: you ask whether I would change error handling across the rest of the code
base over time. That is exactly the concern. Once "this reads better" justifies
rewriting one file, the same argument reaches every file, and you have named
that endpoint yourself.
And the objection I actually raised is still unaddressed: the macros hide the
goto. FD_LINE("...") reads like a plain statement but can jump to a label. For
the people who maintain this code, every exit being visible at the call site is
worth more than the lower visual density. Calling that "objective" or "technical
debt" does not settle it: the mechanism is a tradeoff (C has no exceptions on
purpose), and whether idiomatic working code is "debt" is the very thing in
dispute. Both are judgments the maintainers make.
So I would rather keep session.c as it is. I appreciate the care you put in, and
if you hit a function that is genuinely hard to follow, extracting helper
functions, which keeps the control flow explicit, is something I would look at.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Illia Bobyr
illia-bobyr left a comment (vim/vim#20818)Thanks, this is a thoughtful reply.
Thank you for spending the time to read it.
You are right that session.c mixes three error-handling styles and that separating the happy path from the error path is a real principle. I still have to decline, though, for two reasons.
Scope: you ask whether I would change error handling across the rest of the code base over time. That is exactly the concern. Once "this reads better" justifies rewriting one file, the same argument reaches every file, and you have named that endpoint yourself.
Could you explain a bit more what is wrong here?
If this approach turns out to be good enough to be extended to other files, it means just that.
It is not the "non-functional churn [...] with no natural stopping point."
There is a clear reason and a stopping point.
Just to clarify, I am not saying that I am going to send PRs to change all the other files.
I'm asking about error handling more to understand your view of things.
And, I'm not saying that this approach needs to be spread to all other files.
I don't think myself that it will cover all the cases.
But it seems to work nicely for session.c.
Maybe some other files that do a lot of IO could use it as well, though.
And the objection I actually raised is still unaddressed: the macros hide the goto. FD_LINE("...") reads like a plain statement but can jump to a label. For the people who maintain this code, every exit being visible at the call site is worth more than the lower visual density. Calling that "objective" or "technical debt" does not settle it: the mechanism is a tradeoff (C has no exceptions on purpose), and whether idiomatic working code is "debt" is the very thing in dispute. Both are judgments the maintainers make.
I do not think that just the mere point of something being hidden or visible is bad or good.
For example, when there is code duplication, it is very common to introduce a helper function to remove the duplication.
And the fact that some details are now gone from the call site is not a problem.
I think the situation here is very similar.
There is an enormous number of repeated error checks that provide very little value to the reader.
I must admit that I completely fail to see how is it useful to the maintainers?
In my mind, people who know the codebase the best should be the first to strive to reduce the visual noise and make the happy path more visible.
If one really needs to know how the errors are handled, they just need to check the macro source once.
And every call site is guaranteed to work the same.
This is great for maintainers, as it is easier to make sure that the errors are handled during the review. If a macro is used, it forces an error check.
If you repeat the check at every location, those checks can diverge.
Plus, you need to carefully read each call site during the review.
And it did diverge, as there are already 3 error handling patterns in one file.
This seems to only increase the maintenance burden.
One can certainly get used to a particular pattern being widespread. Kind of creating a "mental macro".
But it hardly seems like a good thing.
It is true that C does not have exceptions or other mechanisms for handling errors.
But it does not mean one cannot implement something that helps.
The same is true for many other things that C is lacking.
Most projects that use C end up implementing a lot of standard data structures, control flow algorithms, and such.
What is wrong with thinking about some error handling patterns?
sessions.c is a chunk of code where there is a lot of precedent for the problem.
By the way, the reason I call it technical debt is not because I want to create an emotional response.
I think that there is a systemic problem with the way error handling is implemented.
Local changes only spread it.
No single local change would fix it, as it would be out of scope.
And would create even more inconsistencies if error handling is done differently.
As long as one thinks that there is a problem with error handling, this matches a technical debt definition for me.
If one is not convinced that error handling can be done better, then it is not technical debt anymore.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20818)Thanks. But once we start accepting "I refactored this to read better" as a
reason to rewrite working code, there is no end to it, and this file is not that
hard to read to begin with.
That is my decision.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.