Vim's 400 line function to wait for keyboard input • Hacker News Board

Re: Vim's 400 line function to wait for keyboard input

#51

Post by haberman » Wed, Jul 15, 2015, 7:06 PM UTC

joosters wrote: post #13

But the code works. It supports lots of platforms through choice. Yes, you could make the code prettier if you dropped some platforms. Yes, you could refactor it to use some abstracting libraries that now exist. But the code works . If you rewrote the code, the best result you could end up with is the same functionality that still works. All other possible results are bad. There is nothing to be gained.

> But the code works.

The "#ifdef maze" in OpenSSL was criticized by the OpenSSH/OpenBSD guys as a key part of the picture that allowed something like Heartbleed to happen: http://blather.michaelwlucas.com/archives/2071

Tons of ifdefs make it much more difficult to ensure that all permutations of flags are correct, or can even compile. I think I remember reading that OpenSSL wasn't even capable of being compiled with the standard, system malloc(). No one compiled it that way, so that build configuration broke without anyone knowing it.

Re: Vim's 400 line function to wait for keyboard input

#52

Post by nkassis » Wed, Jul 15, 2015, 7:07 PM UTC

joosters wrote: post #43

nkassis wrote: post #19

Earlier quoted context omitted.

"There is nothing to be gained." Not completely true, you could argue that it would be more maintainable in the future but I don't think that outweigh the risk of making changes here while it's still fully functional and nothing really needs it to change.

Yes, maintainability / extensibility is good. But I'd be wary of refactoring code purely for the sake of potential maintainability / extensibility, as it's work for no gain. You're trying to predict the future for the code, and that's a losing game. If someone has a reason to extend the code, then you have the choice of hacking in the new feature, or refactoring the code to make it nicer. Each case really needs to be…

I'm OK with that stance, I try to advocate the same myself but you have to be careful of the boiling frog issue of making small incremental refactoring that lead to dead ends.

Stepping back and thinking about the long term is valuable. Doesn't mean you have to do the work now for that potential future but thinking of the potential drawbacks of the current design and it's limitation may guide the decision as to when it's appropriate to do a major refactoring.

Also this particular example seems like something that will probably rarely need modifications. I'd be interested in seeing how often this code changes. My guess is the effort to refactor this will probably be equivalent cost to the future changes.

Re: Vim's 400 line function to wait for keyboard input

#53

Post by omegaham » Wed, Jul 15, 2015, 7:09 PM UTC

x0x0 wrote: post #37

ProAm wrote: post #11

Earlier quoted context omitted.

I really dislike the code review witch hunts you see on HN (or anywhere). On one side of the coin you have startups that merely want to get something thrown together with duct tape, working and ship so they can refactor later and clean things up. On the other hand you have people writing blogs posts to humble brag their code reading and blogging ability. No one knows the circumstances that created this original code.…

A previous employer had a codebase that generated, on a full compilation, at least 10k warnings. That same codebase powered $200mm/year in revenue.

And if your developer time is better spent on producing more features than cleaning up your previously made code, you'll end up with 10.5k warnings. I'm sure that someone will freak out and cry, but as long as you can keep extending it and working with it, why fix it?

Of course, technical debt builds up, and eventually you're badly locked in until you refactor, so it's a balancing act.

Re: Vim's 400 line function to wait for keyboard input

#54

Post by thebelal » Wed, Jul 15, 2015, 7:09 PM UTC

For those interested in viewing this code in context is is at (https://github.com/vim/vim/blob/db9bc0b7931d252cf578c1cd298a...)

Also note that RealWaitForChar has been completely removed from neovim since April of 2014 (https://github.com/neovim/neovim/pull/474/files)

Re: Vim's 400 line function to wait for keyboard input

#55

Post by breadbox » Wed, Jul 15, 2015, 7:09 PM UTC

There is nothing wrong with supporting lots of platforms, but those #ifdefs need to be encapsulated in wrappers functions (or macros). This is a classic example of premature optimization, actually. You use one or two #ifdefs directly because you hate to pay the cost of function overhead just to make the code easier to read. (Even though there's practically no point in tiny optimizations just before the code is going to wait for keyboard input.) A few years go buy, a few more situations are done via #ifdef because at least that way it's consistent. Eventually you have a nightmare function like this, where reading it forces you to read every possible version of the function simultaneously.

Encapsulate your #ifdefs people!

Re: Vim's 400 line function to wait for keyboard input

#56

Post by the_af » Wed, Jul 15, 2015, 7:13 PM UTC

pjmlp wrote: post #30

geofft wrote: post #23

Earlier quoted context omitted.

And your old code doesn't have a test suite you can run on the new code? Well there's your problem. Testless code doesn't acquire bugs by just sitting around on your hard drive, but it doesn't lose any bugs that way, either, and without a test suite you can't afford to do anything but leave it sitting around.

Outside HN like crowds almost no one writes tests. In most enterprises it even has less value than documentation when deadlines approach. The sad reality is that most software is a by product of the main business and as such the quality goals are always pretty low. Edit: typo has => as

I find it odd how many people overlook this fact. I think people reading tech blogs and sites sometimes live in a bubble.

I don't know if in most, but in many companies there are no automated tests at all, and very few "best practices" such as automated builds/continuous integration and even decent source control.

I don't want to name names, but I'll say this: I know for a fact at least one IT/support department in the local branch of a HUGE multinational energy company (guess a few names and you'll get it) doesn't do automated tests of any kind. They are similarly clueless about most things tech-minded folk would consider best practices of the last decade. This department doesn't work on core software, but instead on inventory/procurement systems, but still...

Re: Vim's 400 line function to wait for keyboard input

#57

Post by CephalopodMD » Wed, Jul 15, 2015, 7:20 PM UTC

Camillo wrote: post #7

I'm a bit surprised that they have code like this: if (msec > 0 && ( # ifdef FEAT_XCLIPBOARD xterm_Shell != (Widget)0 # if defined(USE_XSMP) || defined(FEAT_MZSCHEME) || # endif # endif # ifdef USE_XSMP xsmp_icefd != -1 # ifdef FEAT_MZSCHEME || # endif # endif # ifdef FEAT_MZSCHEME (mzthreads_allowed() && p_mzq > 0) # endif )) When they could have written: if (msec > 0 && ( # ifdef FEAT_XCLIPBOARD xterm_Shell != (Wid…

Try to code it, test it, and submit your request. This is why we open source.

Re: Vim's 400 line function to wait for keyboard input

#58

Post by kedean » Wed, Jul 15, 2015, 7:21 PM UTC

jandrese wrote: post #8

This is a classic case of why legacy code is a nightmare to maintain. It's the OpenSSL situation all over again, and one can definitely see the appeal of going through there and ripping out all of the functionality that nobody uses anymore just to make maintenance less of a nightmare. Luckily vim doesn't run suid root on any sane system, so all of this legacy cruft is not as huge of a threat surface on the machine.

It is still an issue, since vi/vim gets used for sudo purposes all the time. Any time someone uses visudo to modify their sudoers file, for example, that's giving root privileges to vim.

Re: Vim's 400 line function to wait for keyboard input

#59

Post by ams6110 » Wed, Jul 15, 2015, 7:21 PM UTC

joosters wrote: post #13

But the code works. It supports lots of platforms through choice. Yes, you could make the code prettier if you dropped some platforms. Yes, you could refactor it to use some abstracting libraries that now exist. But the code works . If you rewrote the code, the best result you could end up with is the same functionality that still works. All other possible results are bad. There is nothing to be gained.

Yes, I don't think this is a WTF. What it really is, is an example of real-world code, that needs to work everywhere, not just in a specific app on a specific OS running on specific hardware... that of course makes it easy for things to be much more tidy.

Re: Vim's 400 line function to wait for keyboard input

#60

Post by joosters » Wed, Jul 15, 2015, 7:27 PM UTC

samf wrote: post #20

joosters wrote: post #13

But the code works. It supports lots of platforms through choice. Yes, you could make the code prettier if you dropped some platforms. Yes, you could refactor it to use some abstracting libraries that now exist. But the code works . If you rewrote the code, the best result you could end up with is the same functionality that still works. All other possible results are bad. There is nothing to be gained.

How do you know it works? Did you test it on every platform? What you would have to gain is maintainability. Maybe it's not worth it; maybe it is. But saying that there is nothing to be gained is just an opinion.

I bet that it contains fewer bugs than a rewrite of the code will have!

That code represents years of tweaks, fixes and obscure workarounds. There are countless problems that you will re-introduce with a code rewrite, because the subtleties in aged, thoroughly-used code are not immediately obvious.