Live data from Hacker News

Vim's 400 line function to wait for keyboard input

geoff.greer.fm

91–100 of 239 posts

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

#91

Earlier quoted context omitted.

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…

Ken Thompson: "And I've always been totally willing to hack things apart if I find a different way that fits better or a different partitioning. I've never been a lover of existing code. Code by itself almost rots and it's gotta be rewritten. Even when nothing has changed, for some reason it rots."

McCoy:

"I know engineers, they love to change things"

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

#92

I assume portability is why the function declaration is old K&R style and not ANSI prototype style. I'm always surprised when I see K&R style in modern(ish) code. I do miss the ability to declare multiple parameters of the same type without repeating the type name, though. Oddly, several modern languages (like D) seem to think that's a feature.

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.

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

#93
post #2

The question is do you think it could have been done better given that they support multiple OS/UI, etc...

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

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

#94
post #37

Earlier quoted context omitted.

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.

Totally in for the balancing act. And additionally in an open source project it's possible to refactor for code beauty, because you can choose so for other reasons than resource management.

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

#95
post #11
post #6

To answer most questions here: Yes, nowadays there are cross-platform libraries you can use instead of implementing that yourself. And yes code that has grown for centuries and contains unnecessary things or patterns that aren't used nowadays can be refactored. Or at least that's how I'd interpret the article.

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

[deleted]

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

#96

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.

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.

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

#98

Earlier quoted context omitted.

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.

You can do whatever you like, but if you were raised right than you'd understand his point. Criticizing something that is broken with no desire/ability/whatever to fix just makes you look like a douche. If you can't do anything about it keep it to yourself; you're just wasting time otherwise.

Wow that's rude. No need to insult someone's upbringing on a thread based on their musings about a snippet of code.

Also you're wrong. Critique is useful in and of itself, not just as a direct means to getting something fixed. If person A doesn't have the time/skill to fix something, they can critique it, hope that person B sees the critique and goes on to fix it. Also, by critiquing it, it starts a discussion about what caused the problem, and how it might be fixed, which is useful for third parties reading the critique: now they know what not to do in their own code.

If you're censoring all of your criticisms just because you don't have the resources to execute on your own, you're hindering discussion and doing a disservice to the community. In a way you're bikeshedding.

Imagine if you were at a coffeeshop and the barista makes a racist remark about the customer in front of you. Do you sit there quietly and let it happen just because you don't have the power to fire him? Or maybe a less loaded example: suppose your friend gets scammed into paying $2000 for a $900 laptop, do you tell him? Doing so might make him feel stupid, but if you care for his well being you'll tell him so that he doesn't do the same thing again.

Criticism isn't rude, it's useful. Insulting someone based on two sentences they post on the internet isn't useful, it's rude.

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

#99
post #67

Earlier quoted context omitted.

> The code works, but it's quite buggy Could you elaborate a bit? I've never experienced a bug related to keyboard input while using Vim, at least that I know of.

Me neither. I've been using vim for about 20 years and I don't think I've ever spotted a single bug in that time.

vim crashes or hangs for me about once every month. I've never bothered to find out why since I don't think it's worth the effort. The effort would be lower if the code was cleaner and didn't have so much bloat related to platforms that don't exist. It would definitely be easier to submit a patch if I didn't need to cater to all the platforms I've never seen.
Post reply on HN