(cache)[SOLVED] Dealing with code blocks with the same code in them

why_bother · 05-05-2025, 01:49 AM

I've written a library that supports both Unix/Linux and Windows, where I have some ifdef's that govern what code gets compiled and built depending on the OS. Where my code makes some system calls, which are obviously OS dependent - hence the need for some ifdef's.

While there isn't anything (else) wrong with it (that I can find), and it's working exactly as intended; Given that Windows and Linux don't have the same system calls; where I check the return value of readdir() and _findfirst(), I have the same code in the event they fail for some reason. While it will still only be included once when you build it (becuase of the ifdef directives), the same code is in two differnet places in the source code (in the if's checking the return value of readdir() and _findfirst()).
While (and as I have with other things) I was thinking of just creating a wrapper and calling that instead; I wondered if there is a better way, or indeed, if it's even worth doing anythng about at all?

I just don't like the same code being in more than one place - even if it'll only be included once in the binary form of it.

So does anyone have any other suggestion to offer, or should I just do the wrapper and be done with it?

Thanks for any thoughts on this in advance.

pan64 · 05-05-2025, 02:08 AM

as an example: https://github.com/RsyncProject/rsyn...ster/syscall.c
(do you mean something like this?)

why_bother · 05-05-2025, 02:18 AM

Quote:

Originally Posted by pan64

as an example: https://github.com/RsyncProject/rsyn...ster/syscall.c
(do you mean something like this?)

Almost. But I've copied and pasted the sections out of my actual code to show you instead of trying to explain it (I did chop out some of the code to make the post shorter).

Code:

#ifdef UNIX
        /****************** START OS DEPENDANT SECTION ***********************/
        /************************ UNIX defined ***********************/

            DIR *baseDirPtr = NULL;
            struct dirent *dir = NULL;

            if (!(baseDirPtr = opendir(current->value))) {
                // code to handle event here
            }

        /****************** END OS DEPENDANT SECTION ***********************/
#endif
#ifdef _WIN64
        /****************** START OS DEPENDANT SECTION ***********************/
        /************************ _WIN64 defined ***********************/

            struct _finddata_t dir;
            register intptr_t fileh;
            char temp[PATH_MAX] = {0};
            HANDLE fh = NULL;
            DWORD access_mask = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | FILE_ALL_ACCESS;
            DWORD grant = 0;

            strcpy(temp, current->value);
            strcat(temp, "\\*");

            if ((fileh = _findfirst(temp, &dir)) == -1L) {
                // directory not found
                // code to handle event here (same code as above)
            }

        /****************** END OS DEPENDANT SECTION ***********************/

Thanks for your help, and quick response.

pan64 · 05-05-2025, 03:32 AM

what I used to do is just put it (duplicated code) into a function. In case of C you can have macros, inlined functions, ....
Or if you prefer the "callback style":

Code:

#if windows
#define read_func read_func_win
#if unix
#define read_func read_func_ux

read(...) {
// some preparation
   ret = read_func(...);
// code to handle event here 
}

but it can be difficult in some cases

why_bother · 05-05-2025, 05:19 AM

I did think about doing a macro for it, but I'm not really in favor of it as: for one thing, you've got to understand the macro to understand the code where it's being used. Also, my C knowledge is far better than my macro knowledge, if I'm being honest.

I do already use inline functions for all of my library's static functions.

But I'll wait and see if anyone else has any other suggestions and pick the option most suitable and probably the simplest option.

Thanks again for your very helpful (and quick) response pan64.

sundialsvcs · 05-05-2025, 09:26 AM

Simply do anything you like that is abundantly clear. To someone who is encountering the code for the first time. Include copious and descriptive comments to make the intention even more clear. (When writing code that even I alone will only see, I am positively "chatty." And many times I have then read "the comments of a complete stranger." That's why they're there. I talk to myself, yack yack yack, and never take them out.)

I will commonly, if possible, write a single "wrapper" function that is expanded in the appropriate way, then call that function everywhere else. It is now clear to the reader of the code how things are supposed to work, and the expansion itself only occurs once. You'll see similar things being done throughout the Linux source code. Clarity is all that matters.

ntubski · 05-05-2025, 05:48 PM

Quote:

Originally Posted by why_bother

Almost. But I've copied and pasted the sections out of my actual code to show you instead of trying to explain it (I did chop out some of the code to make the post shorter).

Based on what you posted, you can't take the check for syscall failure outside of the os-dependent part, because each one uses different variable names. But it seems like that would also prevent the following os-independent code for the successful case from working, so perhaps you've chopped out a bit too much for us to make sense of it?

Quote:

Originally Posted by why_bother

I did think about doing a macro for it, but I'm not really in favor of it as: for one thing, you've got to understand the macro to understand the code where it's being used.

pan64's solution can be written without macros (although probably the READ_FUNC_FAIL_RET definitions would usually be written with macros):

Code:

#if windows
const intptr_t READ_FUNC_FAIL_RET = -1;
intptr_t read_func(...) { // replace void* return type if it makes sense.
   ...
}
#endif // windows
#if unix
DIR*const READ_FUNC_FAIL_RET = NULL;
DIR* read_func(...) {
   ...
}
#endif // unix

read(...) {
   // some preparation
   if (READ_FUNC_FAIL_RET == read_func(...)) {
       // code to handle event here 
   }
}

You could also put the read_func definitions into different files, and link in the appropriate one in your Makefile/build system according to the OS. Then you don't even need to use the preprocessor at all.

why_bother · 05-05-2025, 08:40 PM

Quote:

Originally Posted by ntubski

Based on what you posted, you can't take the check for syscall failure outside of the os-dependent part, because each one uses different variable names. But it seems like that would also prevent the following os-independent code for the successful case from working, ...

Yeah, it only supported Unix/Linux originally - support for Windows was added much later. Although, I hadn't given that a lot of thought either to be honest.
So perhaps that's something to take a closer look at.

Quote:

so perhaps you've chopped out a bit too much for us to make sense of it?

The code I chopped out just calls a couple of functions I wrote to delete the path node from the linked list it uses to store each path, and the other function deletes the path from the database it uses (as it gets metadata about each path and file it sees/finds). Other than that, the only other code there just adjusts the current pointer for the list node, to point it to the next node (if there is one). At the end of the day, the code outside of the two if blocks (for the readdir() and _findfirst() syscalls) works and I don't think it needs to be changed much, if at all (even if I did adopt the suggestions suggested here).

So as far as I can see, I think the suggestions so far look on point to me, so I don't think anyone has misunderstood anything. I can post the code I chopped out, but I'm not sure that's really going to tell anyone anything they haven't already worked out from the code I already posted (other than what I've explained in this post), to be honest.

sundialsvcs · 05-06-2025, 08:04 AM

Logically, the complete functionality would be: "to do something." That consists of issuing a system call and testing for failure. The implementation is OS-specific; the purpose is not. Therefore, I suggest that you "write a purposeful function." Within the implementation of that one function, put the appropriate logic in conditional sections. But, thereafter, call that one function for its purpose.

For instance, look at the Linux source-code that relates to "virtual memory." Every CPU architecture does it a little differently. But you will see that the concepts can be abstracted, and they have been.

ntubski · 05-06-2025, 06:44 PM

Quote:

Originally Posted by why_bother

At the end of the day, the code outside of the two if blocks (for the readdir() and _findfirst() syscalls) works and I don't think it needs to be changed much, if at all (even if I did adopt the suggestions suggested here).

Oh, I missed that you have a variable called "dir" in both alternatives. So you have a common variable to check for the successful path, but not for the failure path. Most obvious solution (smallest change) seems like introducing a common var for the failure case:

Code:

        bool foundDir = false;
#ifdef UNIX
        /****************** START OS DEPENDANT SECTION ***********************/
        /************************ UNIX defined ***********************/

            DIR *baseDirPtr = NULL;
            struct dirent *dir = NULL;

            foundDir = (opendir(current->value) != NULL);

        /****************** END OS DEPENDANT SECTION ***********************/
#endif
#ifdef _WIN64
        /****************** START OS DEPENDANT SECTION ***********************/
        /************************ _WIN64 defined ***********************/

            struct _finddata_t dir;
            register intptr_t fileh;
            char temp[PATH_MAX] = {0};
            HANDLE fh = NULL;
            DWORD access_mask = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | FILE_ALL_ACCESS;
            DWORD grant = 0;

            strcpy(temp, current->value);
            strcat(temp, "\\*");

            foundDir = (_findfirst(temp, &dir) >= 0);
        /****************** END OS DEPENDANT SECTION ***********************/
#endif
        if (!foundDir) {
            // handle event code...
        }

sundialsvcs · 05-06-2025, 08:30 PM

I think that the various alternatives can be as "coupled" as you care for them to be. One "variable in both alternatives," or more than one, or none. Whatever works, and is clear. It must be thoroughly and explicitly tested for all architectures. Write the most-appropriate logic for each case, and comment the hell out of it . . .

why_bother · 05-06-2025, 11:26 PM

Yes, I really do have to agree with your point sundialsvcs. Particularly with what you say in that: the implementation maybe OS dependent (which there isn't much I can do about that - it is what it is), but the purpose is the same, regardless of the OS. So I couldn't agree more with that point. And also, having the relevant comments, particularly where it might not be obvious based on the code why it's doing that, so once again, would have to agree.

That's ok ntubski. I do think you are right in what you say, and things should be consistent for both cases, not just if it works. So I think that's one change that I'll be making. As I prefer it to be consistent, regardless of which OS it's being built for/on.

So I'll see what I can come up with based on both what you guys have suggested, as well as what I was thinking before. I'm thinking that a wrapper function or similar is the way to go here. I'll post back when I've come up with a solution based on what's been said here.

Thanks again for all the suggestions, as they've been very helpful.

sundialsvcs · 05-07-2025, 08:51 PM

I think that "the Linux source-code regarding 'virtual memory'" is an excellent example to follow. First of all, it must now consider more than twenty(!) very different architectures ... and effectively conceal them. It did this using "abstractions."

why_bother · 05-09-2025, 02:43 AM

Quote:

Originally Posted by sundialsvcs

I think that "the Linux source-code regarding 'virtual memory'" is an excellent example to follow. First of all, it must now consider more than twenty(!) very different architectures ... and effectively conceal them. It did this using "abstractions."

While I have looked at *some* Linux kernel source code before; I can't say I'm anything even semi-close to any expert, let alone have I tried to understand how the Linux kernel deals with, and/or allocates memory.

So, do you mind perhaps providing a link to where and what exactly you think I should be looking at? As I'm curious as to why you cite that particular part of the Linux kernel source, and your suggestion in general.

(I do understand what "abstraction" means.)

sundialsvcs · 05-09-2025, 07:24 AM

You can spend as much time as you like probing into this aspect of the Linux kernel – specifically including the many /arch subdirectories. I won't pretend to go into it. But, perhaps the most important thing to see, as you move from the core source-code into the world of /arch, is that each one of them has been separately accommodated. There is a very-clear delineation between the two. (Between: "IBM System/390" and "Commodore Amiga.")