Why version() ?

[Derek Parnell]

On Tue, 10 Feb 2009 17:16:22 -0800, Walter Bright wrote:

> Let's take an example, like the enum for O_XXXX in std.c.linux.linux. 
> Some of those values are common between platforms, and some are unique 
> to particular platforms...

Yes, the duplicated code is a better approach here because the apparent
common code is the key in this example. The lines that are the same are
only coincidently common and and not common due to any inate factors of the
operating system or data being declared.

> This would be even better if the OSX and linux declarations were split 
> into separate "personality" modules. 

Agreed.


The example I offer here is more the sort of thing that it seems you are
also finding when maintaining Phobos ...

Current my code loks like this ...
//--------------- Using version() w/o duplicating code --------

//-------------------------------------------------------
int RunCommand(string pExeName, string pCommand, runCallBack
pCallBack=null)
//-------------------------------------------------------
{

    version(Windows)
    {
        if (std.path.getExt(pExeName).length == 0)
            pExeName ~= ".exe";
    }

    if (util.pathex.IsRelativePath(pExeName) == True)
    {
        string lExePath;
        lExePath = util.pathex.FindFileInPathList("PATH", pExeName);
        if (util.str.ends(lExePath, std.path.sep) == False)
            lExePath ~= std.path.sep;

        version(Windows) pExeName = util.pathex.CanonicalPath(lExePath ~
                                    pExeName, False, False);
        version(Posix)   pExeName = util.pathex.CanonicalPath(lExePath ~
                                    pExeName, False, True);
    }

    if (util.file2.FileExists(pExeName) == false)
    {
        throw new FileExException(std.string.format(
                 "Cannot find application '%s' to run", pExeName));
    }
    return RunCommand( util.str.enquote(pExeName) ~ " " ~ pCommand,
                           pCallBack);
}
//-------------------------------------------------------
int RunCommand(string pCommand, runCallBack pCallBack = null)
//-------------------------------------------------------
{
    int lRC;

    if (pCallBack !is null)
    {
      lRC = pCallBack(1, pCommand, 0);
      if (lRC != 0)
          return 0;
    }

    lRC = system(std.string.toStringz(pCommand));
    version(Posix) lRC = ((lRC & 0xFF00) >> 8);

    if (pCallBack !is null) 
        pCallBack(2, pCommand, lRC);

    return lRC;
}


But if I split the opsys versioning out I get ...


//--------------- Using version() with duplicating code --------
alias int function(int, string, int) runCallBack;
version(Windows)
{
	//-------------------------------------------------------
	int RunCommand(string pExeName, string pCommand, 
                       runCallBack pCallBack=null)
	//-------------------------------------------------------
	{
	
	    if (std.path.getExt(pExeName).length == 0)
	        pExeName ~= ".exe";
	
	    if (util.pathex.IsRelativePath(pExeName) == True)
	    {
	        string lExePath;
	        lExePath = util.pathex.FindFileInPathList("PATH", pExeName);
	        if (util.str.ends(lExePath, std.path.sep) == False)
	            lExePath ~= std.path.sep;
	
	        pExeName = util.pathex.CanonicalPath(lExePath ~ pExeName, False,
                                                     False);
	    }
	
	    if (util.file2.FileExists(pExeName) == false)
	    {
	        throw new FileExException(std.string.format(
                     "Cannot find application '%s' to run", pExeName));
	    }
	    return RunCommand( util.str.enquote(pExeName) ~ " " ~ pCommand,
                                pCallBack);
	}
	//-------------------------------------------------------
	int RunCommand(string pCommand, runCallBack pCallBack = null)
	//-------------------------------------------------------
	{
	    int lRC;
	
	    if (pCallBack !is null)
	    {
	      lRC = pCallBack(1, pCommand, 0);
	      if (lRC != 0)
	          return 0;
	    }
		
	    lRC = system(std.string.toStringz(pCommand));
	
	    if (pCallBack !is null)
	        pCallBack(2, pCommand, lRC);
	    return lRC;
	
	}

}

version(Posix)
{
	//-------------------------------------------------------
	int RunCommand(string pExeName, string pCommand, 
                       runCallBack pCallBack=null)
	//-------------------------------------------------------
	{
	
	    if (util.pathex.IsRelativePath(pExeName) == True)
	    {
	        string lExePath;
	        lExePath = util.pathex.FindFileInPathList("PATH", pExeName);
	        if (util.str.ends(lExePath, std.path.sep) == False)
	            lExePath ~= std.path.sep;
	
	        pExeName = util.pathex.CanonicalPath(lExePath ~ pExeName, False,
                                                     True);
	    }
	
	    if (util.file2.FileExists(pExeName) == false)
	    {
	        throw new FileExException(std.string.format(
                       "Cannot find application '%s' to run", pExeName));
	    }
	    return RunCommand( util.str.enquote(pExeName) ~ " " ~ pCommand,
                               pCallBack);
	}
	//-------------------------------------------------------
	int RunCommand(string pCommand, runCallBack pCallBack = null)
	//-------------------------------------------------------
	{
	    int lRC;
	
	    if (pCallBack !is null)
	    {
	      lRC = pCallBack(1, pCommand, 0);
	      if (lRC != 0)
	          return 0;
	    }
		
	    lRC = system(std.string.toStringz(pCommand));
	    lRC = ((lRC & 0xFF00) >> 8);
	
	    if (pCallBack !is null)
	        pCallBack(2, pCommand, lRC);
	    return lRC;
	
	}

}



Now we have almost twice the code and whenever an enhancement to the
RunCommand function is needed, I must examine and correctly update twice as
many lines now, because *most*, but not quite all, the logic is identical
between both operating systems. This is the sort of scenario that will
cause bugs to be introduced.

The fine line that divides when to duplicate and when not to duplicate, is
hard to see clearly. I tend to favour the less duplication approach, but
only when it leads to lower maintenance costs.

-- 
Derek Parnell
Melbourne, Australia
skype: derek.j.parnell