improvement proposal for json_object_object_foreach

[fhaverkamp]

I tried to use the macro two times within one function and got warnings/errors due to double definitions of key and value. Looking at the current version:

# define json_object_object_foreach(obj,key,val) \
	char *key = NULL; \
	struct json_object *val __attribute__((__unused__)) = NULL; \
	for(struct lh_entry *entry ## key = json_object_get_object(obj)->head, *entry_next ## key = NULL; \
		({ if(entry ## key) { \
			key = (char*)lh_entry_k(entry ## key); \
			val = (struct json_object*)lh_entry_v(entry ## key); \
			entry_next ## key = entry ## key->next; \
		} ; entry ## key; }); \
		entry ## key = entry_next ## key )

it is obvious why this is. I think it would be good idea to wrap the block into a do { } while (0), which I think makes my problem go away:

# define json_object_object_foreach(obj,key,val) \
    do { \
	char *key = NULL; \
	struct json_object *val __attribute__((__unused__)) = NULL; \
	for(struct lh_entry *entry ## key = json_object_get_object(obj)->head, *entry_next ## key = NULL; \
		({ if(entry ## key) { \
			key = (char*)lh_entry_k(entry ## key); \
			val = (struct json_object*)lh_entry_v(entry ## key); \
			entry_next ## key = entry ## key->next; \
		} ; entry ## key; }); \
		entry ## key = entry_next ## key )
    } while (0)

What do you think?

[ploxiln]

That does not work the same, because the macro intends you to provide the body of the for loop right after it, like:

json_object_object_foreach(fruitstand, fruit, q) {
    printf("%s: %d\n", fruit, json_object_get_int(q));
}

The body appearing after a while (0) won't work.

I think this macro should work when nested, if you choose different names for key and val. Can you provide a code sample of your nested object-foreach loops and the compiler warnings/errors you get?

[fhaverkamp]

The loops are not even nested, they are right behind each other. I use the first one to see how many entries in a table I need. After I know that I allocate some memory and use the 2nd loop to fill my new table up. If I would use nested loops, I think the renaming makes totally sense.

The error messages look like this:

cmdserver.c:194:39: error: redefinition of 'key'
        json_object_object_foreach(json_obj, key, val) {
                                             ^
cmdserver.c:172:39: note: previous definition is here
        json_object_object_foreach(json_obj, key, val) {
                                             ^
cmdserver.c:194:44: error: redefinition of 'val'
        json_object_object_foreach(json_obj, key, val) {
                                                  ^
cmdserver.c:172:44: note: previous definition is here
        json_object_object_foreach(json_obj, key, val) {

You are right, I guess the do { } while (0) does not work in this case. I had added it around the foreach blocks such that it created its own block. That way the scope of the variable definitions was separated and the warnings disappeared. I agree that renaming helps to overcome the problem, but I thought that it is much better read and understandable (also copy-paste works better ;-), if multiple of those loops use the same variable names.

Now I tried this:

#define __json_object_object_foreach(obj,key,val)                      \
	do {                                                           \
		json_object_object_foreach(obj,key,val)

#define __json_object_object_foreach_end()                             \
	} while (0)

	/* Count required environment elements */
	nr = 0;
	__json_object_object_foreach(json_obj, key, val) {
		val_type = json_object_get_type(val);

		switch (val_type) {
		case json_type_double:
		case json_type_int:
		case json_type_string:
			nr++;
			break;
		default:
			break; /* Nothing */
		}
	}
	__json_object_object_foreach_end();

That works, but I wonder if I really improved it ;-). Maybe you have a nicer idea?

[ploxiln]

Ah, I see, same problem when not nested.

You can easily use different key and val names, they're macro args for this reason:

foo();

json_object_object_foreach(jobj, key1, val1) {
    bar(key1, bval1);
}
json_object_object_foreach(jobj, key2, val2) {
    bar(key2, val2);
}

If you really want to use the same key and val names, you can create a block scope without any control flow statements, like:

foo();

{
    json_object_object_foreach(json_obj, key, val) {
        bar(key, val);
    }
}

{
    json_object_object_foreach(json_obj, key, val) {
        bar(key, val);
    }
}

Subjectively, I think both these options are better than separate "start" and "end" macros.

[fhaverkamp]

Thanks for the comments. I guess I get along so far. There is another thing I just found when carrying the code over to a Linux box with recent gcc. In my count loop I do not use the key variable.
The compiler complains here about defined and set but not used variable key. So the attribute((unused)) which you already have for the val, might also make sense for the key. I think I will use your macro as example and throw out all I do not need to get my objects counted instead of using the foreach directly.

[dota17]

Problem has been solved and this issue could be closed @hawicz