Peter N. Lundblad wrote:
> Julian,
>
> In r18266, you added a new API svn_wc_revision_status with a pointer to
> struct svn_wc_revision_status_t, which will be filled by the function.
> This avoids lots of "out" parameters in the function, but it still makes
> it impossible to add new fields to the struct without revving both the
> struct and the function (which might be another reason for using a struct
> in the first place).
>
> I suggest making the function allocate the struct and adding to the struct
> doc a note that the size of this struct may increase in the future, so
> callers shouldn't create it themselves. (We can add constructors like
> we've done for other structs now or on demand later).
>
> We've made this mistake before leading to lots of API revision.
Well, here's a patch, but...
I'm not sure that the argument applies to this structure which is used only to
return the result from this one function. If we revise the function, we can't
keep the same signature because although it will be compatible one way
("backward-compatible") it won't be the other way. I'm not quite sure, but I
think the benefit of an extensible structure only arises when it is used in
multiple interfaces.
On the other hand, even if I'm right that this is not giving as much benefit as
multiply-used structures, it still seems better to do it than not. Yes?
For other structures, a "duplicate" function, a.k.a. "copy constructor", was,
as I recall, only needed by the swig/Python bindings or something. Can someone
say if one is likely to be needed here?
- Julian
Tell and enable callers not to rely on the size of the new data structure
svn_wc_revision_status_t, so that we can transparently extend the structure
in future.
* subversion/include/svn_wc.h
(svn_wc_revision_status_t): Note that callers shouldn't rely on its size.
(svn_wc_revision_status): Allocate the result structure.
* subversion/libsvn_wc/revision_status.c
(svn_wc_revision_status): Allocate the result structure.
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 18294)
+++ subversion/include/svn_wc.h (working copy)
@@ -3357,6 +3357,10 @@ svn_error_t *svn_wc_remove_lock (const c
/** A structure to report the mix of revisions found within a working copy,
* and whether any parts are switched or locally modified.
*
+ * @note Fields may be added to the end of this structure in future
+ * versions. Therefore, users shouldn't allocate structures of this
+ * type, to preserve binary compatibility.
+ *
* @since New in 1.4
*/
typedef struct svn_wc_revision_status_t
@@ -3369,29 +3373,34 @@ typedef struct svn_wc_revision_status_t
}
svn_wc_revision_status_t;
-/** Fill @a *result with a summary of the revision range and status of the
- * working copy at @a wc_path (not including "externals").
- *
- * Set @a result->min_rev and @a result->max_rev respectively to the lowest and
- * highest revision numbers in the working copy. If @a committed is true,
- * summarize the last-changed revisions, else the base revisions.
- *
- * Set @a result->switched to indicate whether any item in the WC is switched
- * relative to its parent. If @a trail_url is non-null, use it to determine if
- * @a wc_path itself is switched. It should be any trailing portion of @a
- * wc_path's expected URL, long enough to include any parts that the caller
- * considers might be changed by a switch. If it does not match the end of @a
- * wc_path's actual URL, then report a "switched" status.
+/** Set @a *result_p to point to a new @c svn_wc_revision_status_t structure
+ * containing a summary of the revision range and status of the working copy
+ * at @a wc_path (not including "externals").
+ *
+ * Set @a (*result_p)->min_rev and @a (*result_p)->max_rev respectively to the
+ * lowest and highest revision numbers in the working copy. If @a committed
+ * is true, summarize the last-changed revisions, else the base revisions.
+ *
+ * Set @a (*result_p)->switched to indicate whether any item in the WC is
+ * switched relative to its parent. If @a trail_url is non-null, use it to
+ * determine if @a wc_path itself is switched. It should be any trailing
+ * portion of @a wc_path's expected URL, long enough to include any parts
+ * that the caller considers might be changed by a switch. If it does not
+ * match the end of @a wc_path's actual URL, then report a "switched"
+ * status.
*
- * Set @a result->modified to indicate whether any item is locally modified.
+ * Set @a (*result_p)->modified to indicate whether any item is locally
+ * modified.
*
* If @a cancel_func is non-null, call it with @a cancel_baton to determine
* if the client has cancelled the operation.
*
+ * Allocate *result_p in @a pool.
+ *
* @since New in 1.4
*/
svn_error_t *
-svn_wc_revision_status (svn_wc_revision_status_t *result,
+svn_wc_revision_status (svn_wc_revision_status_t **result_p,
const char *wc_path,
const char *trail_url,
svn_boolean_t committed,
Index: subversion/libsvn_wc/revision_status.c
===================================================================
--- subversion/libsvn_wc/revision_status.c (revision 18294)
+++ subversion/libsvn_wc/revision_status.c (working copy)
@@ -72,7 +72,7 @@ analyze_status (void *baton,
}
svn_error_t *
-svn_wc_revision_status (svn_wc_revision_status_t *result,
+svn_wc_revision_status (svn_wc_revision_status_t **result_p,
const char *wc_path,
const char *trail_url,
svn_boolean_t committed,
@@ -87,6 +87,9 @@ svn_wc_revision_status (svn_wc_revision_
void *edit_baton;
svn_revnum_t edit_revision;
+ svn_wc_revision_status_t *result = apr_palloc (pool, sizeof (**result_p));
+ *result_p = result;
+
/* set result as nil */
result->min_rev = SVN_INVALID_REVNUM;
result->max_rev = SVN_INVALID_REVNUM;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jan 31 00:29:08 2006