Code review comment for ~dirk.zimoch/epics-base:FixShellCommands

Revision history for this message
Andrew Johnson (anj) wrote :

I really don't like adding "#ifdef vxWorks" blocks like this does. We have mechanisms that avoid having such blocks of OS-specific code by putting OS-specific routines in OS-specific source files or by defining OS-specific macros. As a result I can do a git grep '#ifdef vxWorks' on the 7.0 tree and the only hits it finds are in test programs. Also some of the additions here are duplicating code from other routines in the same file, which is another code smell from this merge request.

Maybe we could add a macro to iocsh.h for routines that should be exported for VxWorks, so that for example the dlload command could be implemented like this:

static const iocshFuncDef dlloadFuncDef = {"dlload", 1, dlloadArgs};
IOCSH_STATIC_FUNC void dlload(const char* name)
{
    if (!name || !*name) {
        printf("usage: dlload \"file\"\n");
    }
    else if (!epicsLoadLibrary(name)) {
        printf("epicsLoadLibrary failed: %s\n", epicsLoadError());
    }
}
static void dlloadCallFunc(const iocshArgBuf *args)
{
    dlload(args[0].sval);
}
static void dlloadRegistar(void) {

The IOCSH_STATIC_FUNC macro would expand to static EPICS_ALWAYS_INLINE on everything except VxWorks where it would be empty (Hmm, should GeSys users get these routines as well?). The body of the function is no longer duplicated, there's nothing here which is VxWorks-specific, and if GeSys does need these functions we only have to edit the iocsh.h header to make them available there too.

I'm *not* suggesting that we should have a modules/database/src/ioc/misc/os/vxWorks/dlload.c file, that would be taking this too far and I think the macro solution should work fine.

Sorry to go back on my earlier approval...

review: Needs Fixing

« Back to merge proposal