[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 1. Lets change commit title: the patch does not introduce
> box_space_id_by_name. It was before the patch.
    box: port schema_find_id to C
    
    Part of #3273.

> 2. How can you return -1, when the return type is unsigned?
> 3. It is not 'space_id' speaking in general. This function is used in
> user.cc also to search user id by the name. Please, find another name
> for the parameter.
> 4. "new slab" -> "key". OutOfMemory takes size, alloc function and
> the result variable name.
> 5. You should avoid 'goto cleanup' here.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index c5055ee..a85a09f 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -222,11 +222,11 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
 	trigger_run_xc(&on_alter_space, space);
 }
 
-uint32_t
+int
 schema_find_id(uint32_t system_space_id, uint32_t index_id,
-	       const char *name, uint32_t len, uint32_t *space_id)
+	       const char *name, uint32_t len, uint32_t *object_id)
 {
-	*space_id = BOX_ID_NIL;
+	*object_id = BOX_ID_NIL;
 	if (len > BOX_NAME_MAX) {
 		diag_set(SystemError,
 			 "name length %d is greater than BOX_NAME_MAX", len);
@@ -248,7 +248,7 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
 	uint32_t used = region_used(region);
 	char *key = (char *)region_alloc(region, size);
 	if (key == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "key");
 		return -1;
 	}
 	mp_encode_str(key, name, len);
@@ -261,13 +261,10 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
 	struct tuple *tuple;
 	if (iterator_next(it, &tuple) != 0) {
 		rc = -1;
-		goto cleanup;
-	}
-	if (tuple) {
+	} else if (tuple) {
 		/* id is always field #1 */
-		*space_id = tuple_field_u32_xc(tuple, 0);
+		*object_id = tuple_field_u32_xc(tuple, 0);
 	}
-cleanup:
 	iterator_delete(it);
 	region_truncate(region, used);
 	return rc;

> 6. Same. Do not mention space. It is a function to find ID of
> any system object. Not only space. This function is applicable
> for _index, _space, _user, _func.

diff --git a/src/box/schema.h b/src/box/schema.h
index 099f9b0..cd9bb5b 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -107,16 +107,16 @@ sequence_by_id(uint32_t id);
*
* @param system_space_id identifier of the system space.
* @param index_id identifier of the index to lookup.
- * @param name space to lookup name.
+ * @param name of object to lookup.
* @param len length of a name.
- * @param space_id[out] space_id BOX_ID_NIL - not found.
+ * @param object_id[out] object_id or BOX_ID_NIL - not found.
*
* @retval 0 on success.
* @retval -1 on error.
*/
-uint32_t
+int
schema_find_id(uint32_t system_space_id, uint32_t index_id,
- const char *name, uint32_t len, uint32_t *space_id);
+ const char *name, uint32_t len, uint32_t *object_id);

#if defined(__cplusplus)
} /* extern "C" */