Hello, I was looking at Parrot_get_runtime_prefix as an example of getting environment variable values the parrot way. The function looked over-complicated so I tried a refactor.
The first thing to notice about the function is that it is possible to
leak memory with a call. I did not find any leaks but in checking this
I noticed some API misfeature. This API is built to overload the return
value as either a STRING* or a char*.
Every use in the tree is for a STRING* value, except compilers/immc/main.c
where the char* value is used in a format string right before parrot
terminates.
<snip rev-18443 compilers/imcc/main.c>
case OPT_RUNTIME_PREFIX:
printf("%s\n", Parrot_get_runtime_prefix(interp, NULL));
exit(0);
break;
</snip>
The other user outside of library.c specifically requests a leak-able
char* and then creates a string out of it which is ignorant AFAICT
(is there a real reason for this ?)
<snip rev-18443 src/inter_misc.c>
case RUNTIME_PREFIX:
fullname_c = Parrot_get_runtime_prefix(interp, NULL);
fullname = string_from_cstring(interp, fullname_c, 0);
mem_sys_free(fullname_c);
return fullname;
</snip>
This API function can be made alot simpler both use and implementation wise
by having it return a STRING* every time. The overloading can be easily
replaced by helper functions nesting a call to Parrot_get_runtime_prefix.
I chose the STRING* as it is more useful and robust value to return.
-char*
-Parrot_get_runtime_prefix(Interp *interp, STRING **prefix_str)
+STRING*
+Parrot_get_runtime_prefix (Interp *interp ) {
I am inlining the patch for discussion. It is based against
rev-18443 with a rebase of #41908 applied. If This API
change is acceptable I will rebase it against HEAD clean.
Segway: I mentioned this on #parrot, jisom created a patch that returns
char* instead of STRING*.
It passed the test-suite functionally, I tweaked it a little to pass codingstd
so it is not 100% verified. the Docs test is broken , I haven't played with the
docs yet so they are still out of sync with this prototype of the patch.
--- HEAD/src/library.c 2007-05-06 17:58:47.000000000 -0700
+++ rev-18443/src/library.c 2007-05-07 01:10:27.000000000 -0700
@@ -300,7 +377,7 @@
else
paths = get_search_paths(interp, PARROT_LIB_PATH_INCLUDE);
- Parrot_get_runtime_prefix(interp, &prefix);
+ prefix = Parrot_get_runtime_prefix(interp);
n = VTABLE_elements(interp, paths);
for (i = 0; i < n; ++i) {
path = VTABLE_get_string_keyed_int(interp, paths, i);
@@ -344,60 +425,90 @@
*/
return string_to_cstring(interp, result);
}
-/*
-
-=item C<char* Parrot_get_runtime_prefix(Interp *, STRING **prefix_str)>
-If C<prefix_str> is not NULL, set it to the prefix, else return a malloced
-c-string for the runtime prefix. Remember to free the string with
-C<string_cstring_free()>.
+static STRING*
+query_runtime_prefix ( Interp* interp ) {
-=cut
-
-*/
+ STRING* prefix;
-char*
-Parrot_get_runtime_prefix(Interp *interp, STRING **prefix_str)
-{
- STRING *s, *key;
- PMC *config_hash;
int free_it;
char *env;
env = Parrot_getenv("PARROT_RUNTIME", &free_it);
+
if (env) {
- if (prefix_str) {
- *prefix_str = string_from_cstring(interp, env, 0);
- if (free_it)
- free(env);
- return NULL;
- }
- if (!free_it)
- env = strdup(env);
- return env;
+ prefix = string_from_cstring(interp, env, 0);
+ if (free_it)
+ free(env);
+
+ return prefix;
}
+ return NULL;
+}
+
+static STRING*
+default_runtime_prefix ( Interp* interp ) {
+
+ /* should this be a call to getcwd() ? */
+ const char *pwd = ".";
+
+ return const_string(interp, pwd);
+}
+
+/*
+
+=item C<STRING* Parrot_get_runtime_prefix(Interp * )>
+
+return the runtime prefix in the PMC string C<prefix>. The
+config hash is consulted first, then the environment variable
+PARROT_RUNTIME, and finally a hardcoded internal value is
+returned if that fails.
+=cut
+
+*/
+
+STRING*
+Parrot_get_runtime_prefix (Interp *interp ) {
+
+ PMC *config_hash;
+
+ STRING *key, *can_fail; /* can_fail , for storing string pointers from
+ functions that may fail to return a prefix value
+ */
+
+ /* first look in the config hash for a user specified path */
+
config_hash = VTABLE_get_pmc_keyed_int(interp, interp->iglobals,
(INTVAL) IGLOBALS_CONFIG_HASH);
- key = CONST_STRING(interp, "prefix");
- if (!VTABLE_elements(interp, config_hash)) {
- const char *pwd = ".";
- char *ret;
-
- if (prefix_str) {
- *prefix_str = const_string(interp, pwd);
- return NULL;
+
+ if (VTABLE_elements(interp, config_hash)) {
+ key = CONST_STRING(interp, "prefix");
+ can_fail = VTABLE_get_string_keyed_str(interp, config_hash, key);
+
+ if ( can_fail ) {
+ /*
+ TODO:
+ shouldn't we do some sanity here ? , assuming this can be
+ set by random code/input we should see if it even exists.
+ */
+
+ return can_fail;
}
- ret = (char *)mem_sys_allocate(3);
- strcpy(ret, pwd);
- return ret;
- }
- s = VTABLE_get_string_keyed_str(interp, config_hash, key);
- if (prefix_str) {
- *prefix_str = s;
- return NULL;
}
- return string_to_cstring(interp, s);
+
+ /*
+ fallback:
+
+ no value was found in the config hash so try a system query, if
+ that fails as well return the default.
+ */
+
+ can_fail = query_runtime_prefix(interp);
+
+ return (can_fail)
+ ? can_fail
+ : default_runtime_prefix(interp);
}
/*
--- HEAD/include/parrot/library.h 2007-05-06 17:58:48.000000000 -0700
+++ rev-18443/include/parrot/library.h 2007-05-07 00:15:09.000000000 -0700
@@ -38,7 +38,7 @@
PARROT_API STRING* Parrot_locate_runtime_file_str(Interp *, STRING *file_name,
enum_runtime_ft);
-PARROT_API char* Parrot_get_runtime_prefix(Interp *, STRING **prefix);
+PARROT_API STRING* Parrot_get_runtime_prefix(Interp *);
void parrot_init_library_paths(Interp *);
STRING * parrot_split_path_ext(Interp* , STRING *in,
STRING **wo_ext, STRING **ext);
--- HEAD/compilers/imcc/main.c 2007-04-21 06:34:57.000000000 -0700
+++ rev-18443/compilers/imcc/main.c 2007-05-07 01:10:49.000000000 -0700
@@ -272,7 +273,8 @@
exit(EX_USAGE);
break;
case OPT_RUNTIME_PREFIX:
- printf("%s\n", Parrot_get_runtime_prefix(interp, NULL));
+ printf("%s\n", string_to_cstring(interp,
+
Parrot_get_runtime_prefix(interp)) );
exit(0);
break;
case 'V':
--- HEAD/src/inter_misc.c 2007-04-27 07:00:47.000000000 -0700
+++ rev-18443/src/inter_misc.c 2007-05-07 00:42:27.000000000 -0700
@@ -341,10 +342,7 @@
return basename;
case RUNTIME_PREFIX:
- fullname_c = Parrot_get_runtime_prefix(interp, NULL);
- fullname = string_from_cstring(interp, fullname_c, 0);
- mem_sys_free(fullname_c);
- return fullname;
+ return Parrot_get_runtime_prefix( interp );
default: /* or a warning only? */
internal_exception(UNIMPLEMENTED,
signature.asc
Description: PGP signature
