Retire NETTOOL preprocesor macro?

[janry]

To me switching parts of the code (inside functions) by a macro looks sketchy. Sure it works, and maybe that is the C-way of doing things. But the code can be restructured in such a way that the macro is not needed. This is overall around +-1000 lines change, mostly moving code between files. I can take care of it, but tell me if this is the direction you want to go?

In the attachment there is a first step of this removal of the macro. This is how I would imagine it, multiplied by 10

[prissi]

Nettool is almost not used, so we thought it would be easier to have the same codebase to avoid maintaining extra files which are easily overlooked, e.g. when names change and so on (which happened quite some times during translation latches.)

Using defines is not a problem since modern tools will display the non active parts differently and one can keep the codebase compact.

So hiding a function for other projects is fine. What can be also done with defines but should be avoided, is of course a totally different function for different conditions.

So the macro helps to parse fewer files. Also part like the unpacking depends really on what I want to build, same for the installer. Occasionally, for test builds (like to debug the Android behaviour) I compile them for my windows build. That would be way more difficult, if I had to make new projects just for this test.

So yes, preprecssor macros are "old fashioned" there could be good reasons to use them. (Most libaries are also full of them, if you ever looked under the hood.)

In case of logging for paksets, which is not really needed for the others, putting this function instead into another file could be fine. On the other hand, I tend to avoid touching working code without reasons.

So I guess my answer would very much depends on the function in question. The installation and the loading of pakfiles, and a lot of the event code, I would rather keep in one file.

[janry]

I'm reading that as a "no" to blindly removing the macro in all places. But a "yes" to targeted refactor that improves on using macros for implementing "totally different function for different conditions".

Im proposing that I will split this into many patches over time and you can pick if you approve each one or reject. Each one will leave codebase in consistent and usable state, of course. First one is the 228 in this thread. When proposing future patches I will take into account your guidelines what is good and bad macro usage.

To be honest I don't have clear reason to do that. Just a broad and undefined "developer hunch" - I was working with fuzzer test suite that requires compiling parts of the code into separate "test binary" and somehow I got entangled in the macros and it appeared to me that the task would be easier without needing to understand what exactly NETTOOL macro enables, disables and changes in the flow of the code.

[prissi]

Personally, I think you should save your energy on fixing broken code. Patches just for chaning programming style are unlikely to be done, since anyway there are at least three very different styles of routines in simutrans.

The thing with a project running 25+ years is that every 10 years new dogma and library appear (and somethings fall in derelect). Trends come and go. As long as the code is understandable, it should not matter.

MOreover, the defines in your patch made it so that exactly the same code coudl be used, removing duplication and removing a point of failure. Luckily, not that much is shared between nettool, makobj and simutrans.

[janry]

I'm reading that as "228 won't be merged, and patches to other NETTOOL macro usages probably won't also". That's okay, no worries.

As for the energy - I recharge my energy by doing non-functional changes I consider cleanups (even if other people dont consider them cleanups). Getting them merged is another thing. So if you ever wanted to give me birthday gift or something, then merging a refactor patch is a good candidate.