3479689 New internal routine TclJoinPath().

Refactor all the *Join*Path* routines to give them more useful interfaces
that are easier to manage getting the refcounts right.

6 files changed, 55 insertions, 62 deletions

diff --git a/ChangeLog b/ChangeLog
index 896f9a0..e43d7ac 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2012-01-26  Don Porter  <dgp@users.sourceforge.net>
 
+	* generic/tclCmdAH.c:		New internal routine TclJoinPath().
+	* generic/tclFCmd.c:		Refactor all the *Join*Path* routines
+	* generic/tclFileName.c:	to give them more useful interfaces
+	* generic/tclInt.h:		that are easier to manage getting the
+	* generic/tclPathObj.c:		refcounts right. [Bug 3479689]
+
+2012-01-26  Don Porter  <dgp@users.sourceforge.net>
+
 	* generic/tclPathObj.c:	[Bug 3475569]: Add checks for unshared values
 	before calls demanding them.  [Bug 3479689]: Stop memory corruption
 	when shimmering 0-refCount value to "path" type.
diff --git a/generic/tclCmdAH.c b/generic/tclCmdAH.c
index d036bd6..2308f33 100644
--- a/generic/tclCmdAH.c
+++ b/generic/tclCmdAH.c
@@ -1841,7 +1841,7 @@ PathJoinCmd(
 	Tcl_WrongNumArgs(interp, 1, objv, "name ?name ...?");
 	return TCL_ERROR;
     }
-    Tcl_SetObjResult(interp, Tcl_FSJoinToPath(NULL, objc - 1, objv + 1));
+    Tcl_SetObjResult(interp, TclJoinPath(objc - 1, objv + 1));
     return TCL_OK;
 }
 
diff --git a/generic/tclFCmd.c b/generic/tclFCmd.c
index 0d90094..e95a136 100644
--- a/generic/tclFCmd.c
+++ b/generic/tclFCmd.c
@@ -177,7 +177,6 @@ FileCopyRename(
     for ( ; i<objc-1 ; i++) {
 	Tcl_Obj *jargv[2];
 	Tcl_Obj *source, *newFileName;
-	Tcl_Obj *temp;
 
 	source = FileBasename(interp, objv[i]);
 	if (source == NULL) {
@@ -186,13 +185,11 @@ FileCopyRename(
 	}
 	jargv[0] = objv[objc - 1];
 	jargv[1] = source;
-	temp = Tcl_NewListObj(2, jargv);
-	newFileName = Tcl_FSJoinPath(temp, -1);
+	newFileName = TclJoinPath(2, jargv);
 	Tcl_IncrRefCount(newFileName);
 	result = CopyRenameOneFile(interp, objv[i], newFileName, copyFlag,
 		force);
 	Tcl_DecrRefCount(newFileName);
-	Tcl_DecrRefCount(temp);
 	Tcl_DecrRefCount(source);
 
 	if (result == TCL_ERROR) {
diff --git a/generic/tclFileName.c b/generic/tclFileName.c
index 8ed6f96..adfa2fd 100644
--- a/generic/tclFileName.c
+++ b/generic/tclFileName.c
@@ -787,32 +787,28 @@ Tcl_FSJoinToPath(
     int objc,			/* Number of array elements to join */
     Tcl_Obj *const objv[])	/* Path elements to join. */
 {
-    int i;
-    Tcl_Obj *lobj, *ret;
-
     if (pathPtr == NULL) {
-	lobj = Tcl_NewListObj(0, NULL);
-    } else {
-	lobj = Tcl_NewListObj(1, &pathPtr);
+	return TclJoinPath(objc, objv);
     }
-
-    for (i = 0; i<objc;i++) {
-	Tcl_ListObjAppendElement(NULL, lobj, objv[i]);
+    if (objc == 0) {
+	return TclJoinPath(1, &pathPtr);
     }
-    ret = Tcl_FSJoinPath(lobj, -1);
-
-    /*
-     * It is possible that 'ret' is just a member of the list and is therefore
-     * going to be freed here. Therefore we must adjust the refCount manually.
-     * (It would be better if we changed the documentation of this function
-     * and Tcl_FSJoinPath so that the returned object already has a refCount
-     * for the caller, hence avoiding these subtleties (and code ugliness)).
-     */
+    if (objc == 1) {
+	Tcl_Obj *pair[2];
 
-    Tcl_IncrRefCount(ret);
-    Tcl_DecrRefCount(lobj);
-    ret->refCount--;
-    return ret;
+	pair[0] = pathPtr;
+	pair[1] = objv[0];
+	return TclJoinPath(2, pair);
+    } else {
+	int elemc = objc + 1;
+	Tcl_Obj *ret, **elemv = ckalloc(elemc*sizeof(Tcl_Obj **));
+
+	elemv[0] = pathPtr;
+	memcpy(elemv+1, objv, objc*sizeof(Tcl_Obj **));
+	ret = TclJoinPath(elemc, elemv);
+	ckfree(elemv);
+	return ret;
+    }
 }
 
 /*
diff --git a/generic/tclInt.h b/generic/tclInt.h
index b375bb9..feede54 100644
--- a/generic/tclInt.h
+++ b/generic/tclInt.h
@@ -2995,6 +2995,7 @@ MODULE_SCOPE void	TclInitSubsystems(void);
 MODULE_SCOPE int	TclInterpReady(Tcl_Interp *interp);
 MODULE_SCOPE int	TclIsLocalScalar(const char *src, int len);
 MODULE_SCOPE int	TclIsSpaceProc(char byte);
+MODULE_SCOPE Tcl_Obj *	TclJoinPath(int elements, Tcl_Obj * const objv[]);
 MODULE_SCOPE int	TclJoinThread(Tcl_ThreadId id, int *result);
 MODULE_SCOPE void	TclLimitRemoveAllHandlers(Tcl_Interp *interp);
 MODULE_SCOPE Tcl_Obj *	TclLindexList(Tcl_Interp *interp,
diff --git a/generic/tclPathObj.c b/generic/tclPathObj.c
index 30f2081..7ab8a4e 100644
--- a/generic/tclPathObj.c
+++ b/generic/tclPathObj.c
@@ -838,44 +838,39 @@ Tcl_FSJoinPath(
 				 * reference count. */
     int elements)		/* Number of elements to use (-1 = all) */
 {
-    Tcl_Obj *res;
-    int i;
-    const Tcl_Filesystem *fsPtr = NULL;
+    Tcl_Obj *copy, *res;
+    int objc;
+    Tcl_Obj **objv;
 
-    if (elements < 0) {
-	if (Tcl_ListObjLength(NULL, listObj, &elements) != TCL_OK) {
-	    return NULL;
-	}
-    } else {
-	/*
-	 * Just make sure it is a valid list.
-	 */
-
-	int listTest;
-
-	if (Tcl_ListObjLength(NULL, listObj, &listTest) != TCL_OK) {
-	    return NULL;
-	}
+    if (Tcl_ListObjLength(NULL, listObj, &objc) != TCL_OK) {
+	return NULL;
+    }
 
-	/*
-	 * Correct this if it is too large, otherwise we will waste our time
-	 * joining null elements to the path.
-	 */
+    elements = ((elements >= 0) && (elements <= objc)) ? elements : objc;
+    copy = TclListObjCopy(NULL, listObj);
+    Tcl_ListObjGetElements(NULL, listObj, &objc, &objv);
+    res = TclJoinPath(elements, objv);
+    Tcl_DecrRefCount(copy);
+    return res;
+}
 
-	if (elements > listTest) {
-	    elements = listTest;
-	}
-    }
+Tcl_Obj *
+TclJoinPath(
+    int elements,
+    Tcl_Obj * const objv[])
+{
+    Tcl_Obj *res;
+    int i;
+    const Tcl_Filesystem *fsPtr = NULL;
 
     res = NULL;
 
     for (i = 0; i < elements; i++) {
-	Tcl_Obj *elt, *driveName = NULL;
 	int driveNameLength, strEltLen, length;
 	Tcl_PathType type;
 	char *strElt, *ptr;
-
-	Tcl_ListObjIndex(NULL, listObj, i, &elt);
+	Tcl_Obj *driveName = NULL;
+	Tcl_Obj *elt = objv[i];
 
 	/*
 	 * This is a special case where we can be much more efficient, where
@@ -889,9 +884,8 @@ Tcl_FSJoinPath(
 	if ((i == (elements-2)) && (i == 0)
 		&& (elt->typePtr == &tclFsPathType)
 		&& !((elt->bytes != NULL) && (elt->bytes[0] == '\0'))) {
-	    Tcl_Obj *tailObj;
+	    Tcl_Obj *tailObj = objv[i+1];
 
-	    Tcl_ListObjIndex(NULL, listObj, i+1, &tailObj);
 	    type = TclGetPathType(tailObj, NULL, NULL, NULL);
 	    if (type == TCL_PATH_RELATIVE) {
 		const char *str;
@@ -1389,7 +1383,7 @@ AppendPath(
      * of no evidence that such a foolish thing exists.  This solution was
      * chosen so that "JoinPath" operations that pass through either path
      * intrep produce the same results; that is, bugward compatibility.  If
-     * we need to fix that bug here, it needs fixing in Tcl_FSJoinPath() too.
+     * we need to fix that bug here, it needs fixing in TclJoinPath() too.
      */
     bytes = Tcl_GetStringFromObj(tail, &numBytes);
     if (numBytes == 0) {
@@ -2499,10 +2493,7 @@ SetFsPathFromAny(
 	}
 	Tcl_DStringFree(&temp);
     } else {
-	/* Bug 3479689: protect 0-refcount pathPth from getting freed */
-	pathPtr->refCount++;
-	transPtr = Tcl_FSJoinToPath(pathPtr, 0, NULL);
-	pathPtr->refCount--;
+	transPtr = TclJoinPath(1, &pathPtr);
     }
 
 #if defined(__CYGWIN__) && defined(__WIN32__)