Vim's 400 line function to wait for keyboard input

I wonder why didn't the vim folks write the abstracting library?

>There is nothing to be gained.

What about easier extensibility? Not many are going to want to look at that code, let alone try to add some additional functionality to it.

I am reminded of the old Joel on Software article "Things you should never do part 1": http://www.joelonsoftware.com/articles/fog0000000069.html

From the article:

"The idea that new code is better than old is patently absurd. Old code has been used. It has been tested. Lots of bugs have been found, and they've been fixed. There's nothing wrong with it. It doesn't acquire bugs just by sitting around on your hard drive."

EDIT: Joel isn't actually recommending "do nothing" - on the contrary - the article goes into some depth on ways one can improve software without throwing everything away and starting from scratch.

"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.

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.

In that best result you also end up with code that is smaller and easier to extend in the future. Considering that Vim is still extremely common that is a win.

I made the page as a companion piece to a blog post[1]. The code works, but it's quite buggy. Also, it is almost certainly broken on outdated OSes. It's just that nobody uses the latest Vim on IRIX or VMS, so no bug reports get filed.

A quick glance shows some obvious errors in RealWaitForChar(). For example: with typical preprocessor defines, it uses gettimeofday() in a select() loop. This will break if the system clock changes, either from user intervention or ntpd. The correct solution is to use a monotonically-increasing timer, such as Linux's clock_gettime() or OS X’s mach_absolute_time().

Vim's UI is powerful, but its codebase is a nightmare. My blog post explains why in more detail.

1. http://geoff.greer.fm/2015/01/15/why-neovim-is-better-than-v...

> 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.

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.

Your definition of "works" doesn't include "ability to easily absorb change as the world modernizes". The world of computing is changing each year and any software that works well needs to adapt (or else be trivially small like many unix utils so the world adapts around them or abandons them).

I love vim and have looked at contributing a number of times but the sheer amount of cruft creates a huge barrier.

Actually, best case is you'd have code that still works, but faster, with fewer bugs, and less likelyhood of breaking with future changes. Code working is not the only metric by which code is judged. How WELL it works is more important, for example.

> But the code works.

...is exactly the type of thinking that results is countless of wasted developer time wading through unmaintainable crap, which is never touched because "the code works" and everyone's to damn afraid to change it because it's so excessively complex because no-one bothered to go back and refactor it for readability, maintainability, modern libraries, or anything else.

"working" isn't good enough.

> that still works...

And is more portable, maintainable etc. And criticizing code is not exactly the same thing as saying it should be re-written, given the time investment and low yield.

Yes the code works, but don't write code like this every again.

unreadable code means the code doesn't work for 1 of its 2 purposes : conveying to the reader what it actually does. So it only accomplishes half its job

Not just that, it's just 400 lines. Has the author ever seen production C++ code? This is high-quality easy-to-maintain code in comparison. Come on.

What about being inclusive as an open-source code base? If you simplify the code, remove obsolete cruft and start making use of common place modern libraries, then all of a sudden a bunch of people who were not interested in the first place because the code was way too complicated to get into are suddenly an option. And that's huge.

It's also a case where making that investment is worth it given the core nature of the MSVC standard library which makes it integral to a universe of other software. The global impact of a 5x improvement in something in every app makes it a no-brainer. But there's a lot of debt out there that will be replaced before the payoff ever comes.

Interesting Fermi problem for sadistic interviewers: what would be the global energy saving per invocation of this vim function if a 5x performance improvement were to be found?

Your solution sounds like Enterprise Java to me - instead of understanding one long simple solution, you have to understand 3 shorter, less simple solutions.

How much would that have cost in speed 10, 20 years ago? Function calls aren't free, and their cost was pretty high before Moore's Law all but erased it.

Or don't use them at all. For where the behavior must be different, create different source files with the same function signature. When building, link only with the implementation appropriate for the target being built.

You can easily skip over the #ifdefs - they're all very short.

IMHO I'd rather scroll over code I'm not interested in, than have to jump around to figure out what really happens if they are defined.

I don't think this was done for the purpose of optimization. Usually this happens when you want to add support for one small change, and its easier to get a small patch in vs a big one.

Its not really the classic case of premature optimization either. At least not in my experience. That's more like 'I know I need a spatial partition here, time to research all the ways they can be implemented and their performance tradoffs, and implement a really good one" when you should have just used a hash table and done the other stuff if spatial lookups even show up when profiling.

Stuff like this is way more damaging for both codebase complexity and productivity then anything else. The golden rule is to only do enough optimization to make it easy to do the optimizations you might need to do later, but no more.

But the ifdefs likely negate performance concerns for the most part.

Decomposing this function into different implementations with appropriate abstraction doesn't need to break compatibility.

Also, it just so happens that the people working on NeoVim are currently porting all IO to libuv (the cross-platform library mentioned in the article).

https://github.com/neovim/neovim#whats-being-worked-on-now

It is a good goal for sure.

That said, how does one go about submitting a "fix" for such a widely deployed software? Like, before submitting, I need to make sure it works on Amiga, VMS, Irix... and then all the current platforms. That is, like, a pretty high barrier... who in the world has all these platforms handy for testing? You submit a patch and someone says that it has a problem on Xenix 2.37.4 -- what do you do?

Yep. There's nothing worse that maintaining code that doesn't change, or rarely changes. All that doing nothing because the code works, or works well enough, to avoid anybody noticing it. Yes, that's the classic example of why legacy code is a nightmare to maintain. When I think of maintenance, I think of constantly refactoring things that work.

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.

A slight nit-pick, though I was going to say the exact same thing - `|| 0`. If you do `|| 1` the compiler's going to optimize the entire 'or' expression out ;)

I was thinking that using `0` would cause a problem when you select none of those compilation options, but then I realize the code won't even compile if you don't select at-least one (You get empty parenthesis fallowing the '&&' in the 'if' statement), so it doesn't seem like a big deal.

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

I looked at the spaghetti in your post and assumed it was a joke .. but then I looked at the real code :(

It's been a long time since I've done bare C, but are there now IDEs/editors that let you toggle the define statements you want to activate which then format the code without the ifdef/endif cruft to let you better conceptualize the actual source?

    Open Vim. Type "1.02". Put the cursor on the 2. Hit C-x; get 1.01. Again; get 1.00. Again; get 1.01777777777777777777777.

- Gary Bernhardt (@garybernhardt)

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. The developer may have been working on 10 projects and threw something together just so it'd work.

If it ain't broke....

Doubtful, ifdefs are compile-time

You can refactor it into multiple functions (#ifdefs around a line containing just "||"? seriously??), some of which are no-ops on some platforms / configurations, and let the compiler deal with inlining the functions that exist and optimizing out those that don't. (Not that you would have noticed the overhead anyway, probably.)

The obvious thing to start with is to get rid of MAY_LOOP and arrange things so there's always an infinite loop, and there's always a finished flag, and the finished flag just never gets set to FALSE in the cases where you don't want to loop. Now you've lost several #ifdef...#endif clauses, and there's no #ifdef nesting.

One way of tidying up the inner parts a bit might be by splitting each FD handling code into an init part, and a part that checks for input and a part that checks for error. For example, here's the code for XSMP, whatever that is. This approach is pretty easy, because you can do it with copy and paste. That's exactly how I did it and that's how I can actually present you code:

    #ifdef USE_XSMP
    
    #define InitXSMP()                              \
        if (xsmp_icefd != -1)                       \
        {                                           \
            xsmp_idx = nfd;                         \
            fds[nfd].fd = xsmp_icefd;               \
            fds[nfd].events = POLLIN;               \
            nfd++;                                  \
        }
    
    #define ShouldCheckXSMP() (xsmp_idx >= 0)
    #define IsXSMPInput() (xsmp_idx >= 0 && (fds[xsmp_idx].revents & POLLIN))
    #define IsXSMPError() (xsmp_idx >= 0 && (fds[xsmp_idx].revents & POLLHUP))
    
    #else
    
    #define InitXSMP()                              \
        if (xsmp_icefd != -1)                       \
        {                                           \
            FD_SET(xsmp_icefd, &rfds);              \
            FD_SET(xsmp_icefd, &efds);              \
            if (maxfd < xsmp_icefd)                 \
                maxfd = xsmp_icefd;                 \
        }

    #define ShouldCheckXSMP() (xsmp_icefd != -1)
    #define IsXSMPInput() (xsmp_icefd != -1 && FD_ISSET(xsmp_icefd, &efds))
    #define IsXSMPError() (xsmp_icefd != -1 && FD_ISSET(xsmp_icefd, &rfds))
    
    #endif
    
    #endif

(You could argue about this - for example, should ShouldCheckXSMP() maybe always just be ``(xsmp_icefd!=-1)''? - but the way the code is written, this puts all the details in one place, and the logic in another.)

Then the init code would have this bit:

    #ifdef USE_XSMP
    InitXSMP();
    #endif

And after your poll/select code - which you'd similarly hide in a function or a macro, which I've here assumed sets a flag called `any_events' to say that there were any events that might need looking at - you'd do the business like this:

    #ifdef USE_XSMP
    if (any_events && ShouldCheckXSMP())
    {
        if (IsXSMPInput())
        {
            busy = TRUE;
            xsmp_handle_requests();
            busy = FALSE;

            if (--ret == 0)
                finished = FALSE;   /* keep going if event was only one */
        }
        else if (IsXSMPError())
        {
            if (p_verbose > 0)
                verb_msg((char_u *)_("XSMP lost ICE connection"));
            xsmp_close();
            
            if (--ret == 0)
                finished = FALSE;   /* keep going if event was only one */
        }
    }
    #endif

(usual disclaimers for forum post code apply.)

So: the actual logic is handled in one place, whether or not you're using poll and select, which woud be my key criticism of the code as it stands. And I don't mind having code like this in a #ifdef, if it's only one level deep, particularly if it's somewhat formulaic, which this function would end up being if you approached it this way.

Then repeat for all the parts, and do a bit of work to declare the right variables at the top of the function (something I've just completely ignored).

If you'd prefer to be able to step through it in your average debugger - which tends to do a poor job with #defines - you could do the above with functions, but you'd probably need to move all the state into a struct so that you could pass it around more easily.

Perhaps you could have a mini wrapper for poll and select - for this sort of level of use I'd probably write something local to the file, since it's not so much a separate layer, or a library, or what have you, as just some little helper functions to stop the calling code becoming too awful.

You could always have some kind of extensible function pointer-based system whereby a given descriptor has a callback to be invoked if its FD had an error or has input, which would give you the opportunity to have each subsection of the code supply a low-level function in its own file. (For example, say xsmp_icefd is global only because this function needs to use it - now it could be static to the xsmp support file, which would need only expose a function that would be called from here when input was available or there was an error.)

And so on, and so on. I've worked on this sort of thing quite a lot over the years. There's always a way of doing things that doesn't involve a huge gnarly pile of nested #ifdefs and control structures inside #ifdefs. Either of those, let alone both together, are a good sign that you've taken a wrong turning somewhere.

(Some people are doctrinaire about never including any platform-specific #ifdefs anywhere in the first place. I'm not - but those people definitely do have a point.)

Check vim's style guide (:help style-example in vim), you'll find a note that says:

> NOTE: Don't use ANSI style function declarations. A few people still have to use a compiler that doesn't support it.

> I do miss the ability to declare multiple parameters of the same type without repeating the type name

It's a feature Go has!

Because it is one of the best text editors ever created.

There it is right there.

Please, you can critique something even if you lack the skills to fix it, or the desire to fix it, or the time to fix it.

Also, you can point out that something is broken even if you have no suggestions on how to fix it.

Vim or Emacs, either is fine. So is Eclipse. Etc.

Note Emacs uses http://git.sv.gnu.org/gitweb/?p=gnulib.git to abstract away platforms differences