Re: [vim/vim] session.c: makeopens() is somewhat harder to read due to error checks (PR #20818)

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 or view it on GitHub:
https://github.com/vim/vim/pull/20818#issuecomment-5067350596
You are receiving this because you are subscribed to this thread.

Message ID: <vim/vim/pull/20818/[email protected]>

-- 
-- 
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

--- 
You received this message because you are subscribed to the Google Groups "vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion visit https://groups.google.com/d/msgid/vim_dev/vim/vim/pull/20818/c5067350596%40github.com.