search

hundredrabbits / Orca-c

Live Programming Environment(C Port)
http://wiki.xxiivv.com/orca
MIT License
485 stars 48 forks source link

tui_main: avoid compilation of unused functions when mouse is disabled #69

Closed kylophone closed 4 years ago

kylophone commented 4 years ago

Hi,

Simple patch. Avoids compiling a couple of mouse-only functions when tool is configured with --no-mouse. There were two -Wunused-function compiler warnings which have now been squelched:

⭓  ./tool build --no-portmidi --no-mouse orca
tui_main.c:1654:15: warning: unused function 'ged_mouse_event' [-Wunused-function]
staticni void ged_mouse_event(Ged *a, Usz vis_y, Usz vis_x,

Thanks, Kyle

cancel commented 4 years ago

The master on this GitHub repository was behind the sr.ht one, which means that after this PR had been merged without checking with me 😉 the branches had diverged and the situation was FUBAR. So, a few minutes after the commit was merged, I moved it to its own branch. I brought the GitHub master up to date with the one on sr.ht, and I've fixed it in a more direct way in commit 08e0da9 by simply suppressing the warning. This has the advantage of not requiring adding more ifdefs into the code, thus making it more difficult to work with and check for errors under different build conditions.

kylophone commented 4 years ago

@cancel, a few questions/comments:

neauoire commented 4 years ago

It has been marked as a mirror in https://github.com/hundredrabbits/Orca-c/commit/774d78635745d02f42440e701ae1210ca419784.

cancel commented 4 years ago

BTW, There is another check for FEAT_NOMOUSE in tui_main.c where you will have this problem.

@kylophone where is this? I tested it and I'm not seeing it.

kylophone commented 4 years ago

BTW, There is another check for FEAT_NOMOUSE in tui_main.c where you will have this problem.

@kylophone where is this? I tested it and I'm not seeing it.

https://github.com/hundredrabbits/Orca-c/blob/master/tui_main.c#L3557-L3579

cancel commented 4 years ago

https://github.com/hundredrabbits/Orca-c/blob/master/tui_main.c#L3557-L3579

I'm not sure what the problem is here?

cancel commented 4 years ago

Not sure I like your approach of always compiling/linking these 2 unused functions, but I guess I get it.

They are marked as static, so they do not have external linkage. It's a few hundred bytes of instructions, maybe. Dead code elimination is enabled in release builds so that those bytes won't disturb you.

kylophone commented 4 years ago

Not really a problem, just pointing out that your concern about potential compilation errors under different build conditions exists here too. That was the rationale for avoiding 798bd69 and compiling the unused functions.

cancel commented 4 years ago

The ifdef has to be handled somewhere, though.

cancel commented 4 years ago

Ah OK, you just mean the problem exists in general? Well, yeah. There's not much that can be done about that. I'm mostly trying to reduce the number of ifdef'd code sections within reason. One of the main things I'm trying to avoid is having code disabled unbeknownst to some other person working on the code, and they break it but it never gets checked because it was disabled, and they submit a patch under that condition.

Another thing I try to avoid is making it difficult to navigate the code. If you're using a simple editor like vim, it's hard to tell where ifdef'd blocks begin and end. And it can make it harder to rearrange code and be confident that you didn't screw it up without testing all possible combinations of flags. (Fortunately, orca-c also doesn't have many of them.)

For both of those things, I think the tradeoff of maybe spending an extra couple of milliseconds on compilation is worth it in this case.

If it was a large amount of code, or code that was isolated in some way and not interleaved into a longer file, and if orca had some more sophisticated automatic testing system, maybe it would make sense to disable it.