Here's my review. It might be a bit incoherent in places, because I
dind't write it top-down, and I only thought of using a temporary pool
for expansion towards the end. So, read with a grain of salt. :-)
Daniel Rall wrote:
>* subversion/libsvn_subr/config.c
> (DEFAULT_SECTION): The name of the default configuration section.
>
I think you want to put that in config_impl.h, because config_win.c will
need it, too (it doesn't have a special default section; instead, it
relies on Registry structure to fill it in, and will have to use the
same name you do.
> (find_option): If the option in the specified section isn't found,
> and look in the default section for an option of the same name (if
> we aren't already looking there).
>
There's something missing between "found," and "and".
> (make_string_from_option): Call expand_option_value(), and set
> opt->expanded. Expanded signature to take a cfg_section_t pointer
> for use by expand_option_value()/find_option().
>
> (FMT_START, FMT_END): Constants used in expand_option_value().
>
> (expand_option_value): New function which expands option values. It
> takes pointers to a value and x_value string -- rather than
> directly to a cfg_option_t -- so that it can be called on plain
> text (as on the default value parameter passed to
> svn_config_get()). It must be forward decl'd ahead of
> make_string_from_option(), as the functions have a bi-directional
> dependency upon each other.
>
> A number of questions about this function:
> - Should it handle printf-style fancy formatting? Non-string
> formats?
>
Huh? I don't see what you could do with those. All you have is the mapping
%(foo)s -> get_option(this_section, foo)
Remember, in ConfigParser, the values can be any kind of object (if you
create them programatically). In our case, they can only be strings.
> - How should conditions which would be exceptions to Python's
> ConfigParser be handled: specifically, the failure to resolve an
> expansion, and an unterminated format string?
>
If the placeholder is not terminated, it's not a placeholder, therefore
you leave it alone. If it cannot be expanded, you also leave it alone
(i.e., you *don't* replace it with an empty string, or anything).
> - What's the Right Way to get the c-string data out of a
> svn_stringbuf_t? I'd like to "free" the structure after copying
> its data (I'm done with it at that point and don't see any
> reason for it to stick around in the pool).
>
You can't free anything from pools. What you can do is us the
svn_config_t's tmp_key stringbuf to build up the value, then
apr_pstrmemdup(cfg->x_pool, cfg->tmp_key->data, cfg->tmp_key->len). That
way you'll reuse tmp_key all the time (but note that you have to
document this use, too).
On second thought, add a tmp_value stringbuf to the svn_config_t, so
that tmp_key is always used for names and tmp_value for values. We don't
want them to bash on each other by mistake. And remembet to always use
svn_config_t::pool when manipulating svn_config_t::tmp_key and
svn_config_t::tmp_value, never x_pool.
> (svn_config_get): Pass a cfg_section_t ** to find_option() so the
> section is available when make_string_from_option() is called.
> Call expand_option_value() on the default_value parameter.
>
> (svn_config_enumerate): Pass a cfg_section_t * to
> make_string_from_option() to handle the function's API change.
>
>* subversion/tests/libsvn_subr/config-test.c
> Test expansion behavior of the svn_config_read() and
> svn_config_get() functions.
>
> (config_keys, config_values): More name/value pairs from the
> configuration file.
>
> (test1): Added a conditionally compiled debugging trace. Fail the
> test if only one of the values is null.
>
>* subversion/tests/libsvn_subr/config-test.cfg
> Introduced more features from Python's ConfigParser file format into
> test file.
>
>
>Index: subversion/libsvn_subr/config.c
>===================================================================
>--- subversion/libsvn_subr/config.c (revision 5277)
>+++ subversion/libsvn_subr/config.c (working copy)
>@@ -340,6 +340,8 @@
> }
>
>
>+static const char *DEFAULT_SECTION = "DEFAULT";
>+
>
Just FYI, this idiom is usually implemented as an array, not as a char
pointer:
static const char DEFAULT_SECTION[] = "DEFAULT";
But you'll be moving this into cofig_impl.h, so instead you'll get
#define SVN_CONFIG__DEFAULT_SECTION "DEFAULT"
> /* Return a pointer to an option in CFG, or NULL if it doesn't exist.
> if SECTIONP is non-null, return a pointer to the option's section.
> OPTION may be NULL. */
>@@ -361,26 +363,38 @@
> if (sec_ptr != NULL && option != NULL)
> {
> cfg_section_t *sec = sec_ptr;
>+ cfg_option_t *opt;
>
> /* Canonicalize the option key */
> svn_stringbuf_set (cfg->tmp_key, option);
> make_hash_key (cfg->tmp_key->data);
>
>- return apr_hash_get (sec->options, cfg->tmp_key->data,
>- cfg->tmp_key->len);
>+ opt = apr_hash_get (sec->options, cfg->tmp_key->data,
>+ cfg->tmp_key->len);
>+ /* NOTE: ConfigParser's sections are case sensitive. */
>
Yes, I think we document elsewhere that case-insensitivity in the
section and option names is (should be) the only difference between our
svn_config_t and ConfigParser.
>+ if (opt == NULL && apr_strnatcasecmp(section, DEFAULT_SECTION) != 0)
>+ /* Options which aren't found in the requested section are
>+ also sought after in the default section. */
>+ opt = find_option (cfg, DEFAULT_SECTION, option, &sec);
>+ return opt;
> }
>
> return NULL;
> }
>
>
>+static void
>+expand_option_value (svn_config_t *cfg, cfg_section_t *section,
>+ const char *opt_value, const char **opt_x_valuep);
>+
>+
> /* Set *VALUEP according to the OPT's value. */
> static void
>-make_string_from_option (const char **valuep,
>- svn_config_t *cfg, cfg_option_t *opt)
>+make_string_from_option (const char **valuep, svn_config_t *cfg,
>+ cfg_section_t *section, cfg_option_t *opt)
> {
>- /* ### TODO: Expand the option's value */
>- (void)(cfg);
>+ expand_option_value (cfg, section, opt->value, &opt->x_value);
>+ opt->expanded = TRUE;
>
> /* For legacy reasons, the cfg is still using counted-length strings
> internally. But the public interfaces just use null-terminated
>@@ -393,6 +407,78 @@
> }
>
>
>+#define FMT_START "%("
>+#define FMT_END ")s"
>
>+
>+
>+static void
>+expand_option_value (svn_config_t *cfg, cfg_section_t *section,
>+ const char *opt_value, const char **opt_x_valuep)
>+{
>+ svn_stringbuf_t *buf = NULL;
>
Just use cfg->tmp_val instead.
>+ const char *remaining_value = opt_value;
>+ const char *name_start, *name_end;
>+
>+ /* ### Handle fancy/non-string format specifiers? */
>
Nope. :-)
>+ while (remaining_value != NULL
>+ && (name_start = strstr (remaining_value, FMT_START)) != NULL)
>+ {
>+ name_start += strlen (FMT_START);
>
Don't do that in a loop. Either compute this strlen once outside the
loop, or use
sizeof (FMT_START) - 1
That's compile-time and will work just fine.
>+ name_end = strstr (name_start, FMT_END);
>+
>+ if (name_end != NULL)
>+ {
>+ cfg_option_t *x_opt;
>+ int name_len = name_end - name_start;
>+ char name[name_len + 1];
>
Excuse me? This actually compiles on your box? Variable-length arrays
and late declarations are a C99 feature. We can't use C99 in our code,
because there aren't enough C99 compilers in the world. You can't do that.
You need yet another temporary stringbuf in svn_config_t, I think.
>+ apr_cpystrn (name, name_start, name_len + 1);
>
So:
svn_stringbuf_ensore(cfg->tmp_opt, name_len + 1);
apr_cpystrn(cfg->tmp_opt->data, name_start, name_len + 1);
>+ x_opt = find_option (cfg, section->name, name, NULL);
>+
>+ if (x_opt != NULL)
>+ {
>+ const char *cstring;
>+ make_string_from_option (&cstring, cfg, section, x_opt);
>
I think there's a serious problem here. You're calling
make_string_from_option recursively. Instead, you should just insert the
expanded value in-place, and continue from the beginning of the
expansion. This means changing the way you're handling origiinal value.
Hm. this means that using cfg->tmp_val and cfg->tmp_opt won't work.
Yikes. Tell you what, just use a temporary pool in
make_string_from_option, do all your temporary allocations from there,
then destroy the pool after expansion. Your make_string_from_option then
becomes:
make_string_from_option (const char &&valuep, svn_config_t *cfg,
cfg_section_t *sec, cfg_option_t *opt,
apr_pool_t *x_pool)
{
apr_pool_t *temp_pool = (x_pool ? x_pool : svn_pool_create(cfg->x_pool));
...
if (!x_pool)
svn_pool_destroy(temp_pool);
}
Then you pass temp_pool as a new parameter to expand_option_value, and
pass /that/ pool back to make_string_from_option. That way you'll use
the same pool for any number of recursive expansions, and clear it at
the end (by that time, you shouldn've copied the result into x_pool, if
necessary).
>+
>+ if (buf == NULL)
>+ {
>+ int n = name_start - strlen (FMT_START) - remaining_value;
>+ buf = svn_stringbuf_ncreate(remaining_value, n,
>+ cfg->x_pool);
>+ cfg->x_values = TRUE;
>+ }
>+ svn_stringbuf_appendcstr (buf, cstring);
>
Don't use svn_stringbuf_appendcstr, use svn_stringbuf_appendbytes
instead, so that you don't have to copy the original to get null
termination.
>+ remaining_value = name_end + strlen(FMT_END);
>
Again, don't strlen a constant string in a loop.
>+ name_start = strstr (remaining_value, FMT_START);
>+ if (name_start == NULL)
>+ {
>+ /* Done looping, no more expansions to process. */
>+ svn_stringbuf_appendcstr (buf, remaining_value);
>+ remaining_value = NULL;
>+ }
>+ }
>+ else
>+ /* Though ConfigParser considers the failure to resolve
>+ the requested expansion an exception condition, we
>+ leave the expanded value set to NULL. */
>+ remaining_value = NULL;
>
No, you shouldn't terminate the loop here. You should just skip the
placeholder and look for the next one.
>+ }
>+ else
>+ /* Though ConfigParser treats unterminated format specifiers
>+ as an exception condition, we leave the expanded value set
>+ to NULL. */
>+ remaining_value = NULL;
>
Same here.
>+ }
>+
>+ if (buf != NULL)
>+ {
>+ /* ### HELP: What to do with buf? We want its c-string data,
>+ ### not the struct itself. */
>+ *opt_x_valuep = buf->data;
>
*opt_x_valuep = apr_pstrmemdup(cfg->x_pool, buf->data, buf->len);
I think we already document that what you get from svn_config_get is
only valid until the next call to any svn_config function.
Hm, it's not so simple, really -- you should only copy to the
cfg->x_pool at the end of a complete expansion. Remember, you have a
pair recursion between make_string_from_option and expand_option_value.
Only the top call to expand_option_value should copy the data to
cfg->x_pool. Alernatively, you could have the top
make_string_from_option do that *if* the value got expanded (there, you
at least know you're at the top of the recursoin, because my proposed
x_pool parameter will be NULL).
>+ }
>+}
>+
>+
>
> void
> svn_config_get (svn_config_t *cfg, const char **valuep,
>@@ -401,11 +487,12 @@
> {
> if (cfg)
> {
>- cfg_option_t *opt = find_option (cfg, section, option, NULL);
>+ cfg_section_t *sec;
>+ cfg_option_t *opt = find_option (cfg, section, option, &sec);
> if (opt != NULL)
>- make_string_from_option (valuep, cfg, opt);
>+ make_string_from_option (valuep, cfg, sec, opt);
> else
>- *valuep = default_value; /* ### TODO: Expand default_value */
>+ expand_option_value (cfg, sec, default_value, valuep);
> }
> else
> *valuep = default_value;
>@@ -481,7 +568,7 @@
> opt = opt_ptr;
>
> ++count;
>- make_string_from_option (&temp_value, cfg, opt);
>+ make_string_from_option (&temp_value, cfg, sec, opt);
> if (!callback (opt->name, temp_value, baton))
> break;
> }
>Index: subversion/tests/libsvn_subr/config-test.cfg
>===================================================================
>--- subversion/tests/libsvn_subr/config-test.cfg (revision 5277)
>+++ subversion/tests/libsvn_subr/config-test.cfg (working copy)
>@@ -1,3 +1,24 @@
>+# default values across all sections
>+[DEFAULT]
>+foo=bar
>+# Not implementing __name__ expansions
>+#baz=%(__name__)s
>+
> [section1]
>+# Trailing whitespace
> a=Aa
>-b= 100
>\ No newline at end of file
>+# leading whitespace / numeric
>+b= 100
>+# Variable expansion
>+c=%(foo)s
>+# Expansion for non-existent option (ConfigParser throws an
>+# InterpolationError with the message "Bad value substitution")
>+d=%(bogus)s
>+# Expansion format escaping doesn't seem possible
>+e=%%(a)s
>+# Two-level variable expansion with surrounding text
>+f=lyrical %(c)sd
>+# Unterminated format string
>+g= %(unterminated
>+# Multiple expansions
>+h=%(a)s %(b)s
>\ No newline at end of file
>Index: subversion/tests/libsvn_subr/config-test.c
>===================================================================
>--- subversion/tests/libsvn_subr/config-test.c (revision 5277)
>+++ subversion/tests/libsvn_subr/config-test.c (working copy)
>@@ -84,10 +84,12 @@
> }
>
>
>-static const char *config_keys[] = { "a", "b", NULL };
>-static const char *config_values[] = { "Aa", "100", NULL };
>+static const char *config_keys[] = { "foo", "a", "b", "c", "d", "e", "f", "g",
>+ "h", NULL };
>+static const char *config_values[] = { "bar", "Aa", "100", "bar", "%(bogus)s",
>+ "%Aa", "lyrical bard",
>+ "%(unterminated", "Aa 100", NULL };
>
>-
> static svn_error_t *
> test1 (const char **msg,
> svn_boolean_t msg_only,
>@@ -117,7 +119,14 @@
> py_val = (char *) config_values[i];
> svn_config_get(cfg, (const char **) &c_val, "section1", key,
> "default value");
>- if (c_val != NULL && py_val != NULL && strcmp(c_val, py_val) != 0)
>+#if 0
>+ printf("Testing expected value '%s' against '%s' for "
>+ "option '%s'\n", py_val, c_val, key);
>+#endif
>+ /* Fail iff one value is null, or the strings don't match. */
>+ if ((c_val == NULL ^ py_val == NULL)
>+ || (c_val != NULL && py_val != NULL
>+ && apr_strnatcmp(c_val, py_val) != 0))
> return fail(pool, "Expected value '%s' not equal to '%s' for "
> "option '%s'", py_val, c_val, key);
> }
>
>
>
>
--
Brane Čibej <brane_at_xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Mar 11 22:41:26 2003