search

9fans / plan9port

Plan 9 from User Space
https://9fans.github.io/plan9port/
Other
1.61k stars 319 forks source link

winwatch: fix double free bug, simplify error handling, reduce X11 calls #578

Closed markvanatten closed 1 year ago

markvanatten commented 1 year ago

A double free bug caused winwatch to crash sometimes upon closing a window. The setjmp in the X error handler was not necessary. The many XFlush and XSync calls, together with the surrounding changes of X error handler, were not necessary.

dancrossnyc commented 1 year ago

This change sounds great, but there are a number of style changes that a) obscure the main contentful changes, and b) go against plan9 C style; if those parts of the patch could be reverted and the commit message could be prefixed with, winwatch: that'd be great.

markvanatten commented 1 year ago

I'll be happy to, but should like to ask your advice on C style. I looked at the Plan 9 source for rio, and at Plan 9's man page on style(6), but may well have missed things.

markvanatten commented 1 year ago

This commit modifies just the functionality. Editing for style will be done in a further commit. Please advise on stylistic conventions. E.g. in surrounding '==' with spaces I was following the advice on binary operators in Plan 9's style(6).

dancrossnyc commented 1 year ago

This commit modifies just the functionality. Editing for style will be done in a further commit. Please advise on stylistic conventions. E.g. in surrounding '==' with spaces I was following the advice on binary operators in Plan 9's style(6).

Thanks.

Re: style changes, as unsatisfying as it is, in general I would simply recommend against them for code of this vintage.

The, perhaps sad, fact is that Plan 9 style had very little mechanical support (the cb command exists, but didn't seem terribly widely used) and was only loosely adhered to. I've found that large projects that do not strictly adhere to stylistic rules very quickly find those rules devolving into loose guidelines, and ex post facto changes to bring code into style adherence become noisy churn or (worse) inadvertently introduce bugs.

For better or worse, the best strategy for dealing with plan9 code generally is to avoid the temptation to make stylistic changes unless they result in clarifying misleading code (for example, extra indentation that would lead one to believe that a line is under the control of some conditional when it is not). The best advice is to mimic the existing code.

Perhaps some day we'll bite the bullet and adopt some tool to automate style enforcement, and then we'll blanket run it across the source base, but I doubt that'll happen any time particularly soon.