Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:
diff --git a/config.c b/config.c
index f55c43d..fe1c0e5 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,42 @@
#include strbuf.h
#include quote.h
-typedef struct config_file {
- struct config_file *prev;
- FILE *f;
+struct config_source {
+ struct config_source *prev;
+ void *data;
Would a union be more appropriate here? We do not ever have to pass it
directly as a parameter, since we pass the struct config_source to the
method functions.
It's still possible to screw up using a union, but it's slightly harder
than screwing up using a void pointer. And I do not think we need the
run-time flexibility offered by the void pointer in this case.
+static int config_file_fgetc(struct config_source *conf)
+{
+ FILE *f = conf-data;
+ return fgetc(f);
+}
This could become just:
return fgetc(conf-u.f);
and so forth (might it make sense to give f a more descriptive name,
as we are adding other sources?).
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
On Tue, Mar 12, 2013 at 07:03:55AM -0400, Jeff King wrote:
On Sun, Mar 10, 2013 at 05:59:40PM +0100, Heiko Voigt wrote:
diff --git a/config.c b/config.c
index f55c43d..fe1c0e5 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,42 @@
#include strbuf.h
#include quote.h
-typedef struct config_file {
- struct config_file *prev;
- FILE *f;
+struct config_source {
+ struct config_source *prev;
+ void *data;
Would a union be more appropriate here? We do not ever have to pass it
directly as a parameter, since we pass the struct config_source to the
method functions.
It's still possible to screw up using a union, but it's slightly harder
than screwing up using a void pointer. And I do not think we need the
run-time flexibility offered by the void pointer in this case.
No we do not need the void pointer flexibility. But that means every new
source would need to add to this union. Junio complained about that fact
when I first added the extra members directly to the struct. A union
does not waste that much space and we get some seperation using the
union members. Since this struct is local only I think that should be
ok.
+static int config_file_fgetc(struct config_source *conf)
+{
+ FILE *f = conf-data;
+ return fgetc(f);
+}
This could become just:
return fgetc(conf-u.f);
and so forth (might it make sense to give f a more descriptive name,
as we are adding other sources?).
Will change that.
Cheers Heiko
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] config: make parsing stack struct independent from actual data source
On Tue, Mar 12, 2013 at 05:27:35PM +0100, Heiko Voigt wrote: Would a union be more appropriate here? We do not ever have to pass it directly as a parameter, since we pass the struct config_source to the method functions. It's still possible to screw up using a union, but it's slightly harder than screwing up using a void pointer. And I do not think we need the run-time flexibility offered by the void pointer in this case. No we do not need the void pointer flexibility. But that means every new source would need to add to this union. Junio complained about that fact when I first added the extra members directly to the struct. A union does not waste that much space and we get some seperation using the union members. Since this struct is local only I think that should be ok. Right. I think that is not a big deal, as we are not exposing this struct outside of the config.c; any additions would need to add a new git_config_from_foo function, anyway. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] config: make parsing stack struct independent from actual data source
To simplify adding other sources we extract all functions needed for
parsing into a list of callbacks. We implement those callbacks for the
current file parsing. A new source can implement its own set of callbacks.
Instead of storing the concrete FILE pointer for parsing we store a void
pointer. A new source can use this to store its custom data.
Signed-off-by: Heiko Voigt hvo...@hvoigt.net
---
config.c | 63 +++
1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/config.c b/config.c
index f55c43d..fe1c0e5 100644
--- a/config.c
+++ b/config.c
@@ -10,20 +10,42 @@
#include strbuf.h
#include quote.h
-typedef struct config_file {
- struct config_file *prev;
- FILE *f;
+struct config_source {
+ struct config_source *prev;
+ void *data;
const char *name;
int linenr;
int eof;
struct strbuf value;
struct strbuf var;
-} config_file;
-static config_file *cf;
+ int (*fgetc)(struct config_source *c);
+ int (*ungetc)(int c, struct config_source *conf);
+ long (*ftell)(struct config_source *c);
+};
+
+static struct config_source *cf;
static int zlib_compression_seen;
+static int config_file_fgetc(struct config_source *conf)
+{
+ FILE *f = conf-data;
+ return fgetc(f);
+}
+
+static int config_file_ungetc(int c, struct config_source *conf)
+{
+ FILE *f = conf-data;
+ return ungetc(c, f);
+}
+
+static long config_file_ftell(struct config_source *conf)
+{
+ FILE *f = conf-data;
+ return ftell(f);
+}
+
#define MAX_INCLUDE_DEPTH 10
static const char include_depth_advice[] =
exceeded maximum include depth (%d) while including\n
@@ -172,13 +194,12 @@ static int get_next_char(void)
c = '\n';
if (cf) {
- FILE *f = cf-f;
- c = fgetc(f);
+ c = cf-fgetc(cf);
if (c == '\r') {
/* DOS like systems */
- c = fgetc(f);
+ c = cf-fgetc(cf);
if (c != '\n') {
- ungetc(c, f);
+ cf-ungetc(c, cf);
c = '\r';
}
}
@@ -339,7 +360,7 @@ static int get_base_var(struct strbuf *name)
}
}
-static int git_parse_file(config_fn_t fn, void *data)
+static int git_parse_source(config_fn_t fn, void *data)
{
int comment = 0;
int baselen = 0;
@@ -896,7 +917,7 @@ int git_default_config(const char *var, const char *value,
void *dummy)
return 0;
}
-static int do_config_from(struct config_file *top, config_fn_t fn, void *data)
+static int do_config_from_source(struct config_source *top, config_fn_t fn,
void *data)
{
int ret;
@@ -908,7 +929,7 @@ static int do_config_from(struct config_file *top,
config_fn_t fn, void *data)
strbuf_init(top-var, 1024);
cf = top;
- ret = git_parse_file(fn, data);
+ ret = git_parse_source(fn, data);
/* pop config-file parsing state stack */
strbuf_release(top-value);
@@ -925,12 +946,15 @@ int git_config_from_file(config_fn_t fn, const char
*filename, void *data)
ret = -1;
if (f) {
- config_file top;
+ struct config_source top;
- top.f = f;
+ top.data = f;
top.name = filename;
+ top.fgetc = config_file_fgetc;
+ top.ungetc = config_file_ungetc;
+ top.ftell = config_file_ftell;
- ret = do_config_from(top, fn, data);
+ ret = do_config_from_source(top, fn, data);
fclose(f);
}
@@ -1063,7 +1087,6 @@ static int store_aux(const char *key, const char *value,
void *cb)
{
const char *ep;
size_t section_len;
- FILE *f = cf-f;
switch (store.state) {
case KEY_SEEN:
@@ -1075,7 +1098,7 @@ static int store_aux(const char *key, const char *value,
void *cb)
return 1;
}
- store.offset[store.seen] = ftell(f);
+ store.offset[store.seen] = cf-ftell(cf);
store.seen++;
}
break;
@@ -1102,19 +1125,19 @@ static int store_aux(const char *key, const char
*value, void *cb)
* Do not increment matches: this is no match, but we
* just made sure we are in the desired section.
*/
- store.offset[store.seen] = ftell(f);
+ store.offset[store.seen] = cf-ftell(cf);
/* fallthru */
case SECTION_END_SEEN:
case START:
if (matches(key, value)) {
- store.offset[store.seen] = ftell(f);
+ store.offset[store.seen] = cf-ftell(cf);
