BLI: Add std::string variant of BLI_uniquename_cb #114052

Sergey Sharybin · 2023-10-23T10:03:08+02:00

Sergey Sharybin commented

2023-10-23 10:03:08 +02:00

Allows to ensure unique name for cases when name is a dynamically
sized string.

Allows to ensure unique name for cases when name is a dynamically sized string.

Sergey Sharybin added 1 commit 2023-10-23 10:03:18 +02:00

5f8f136640 BLI: Add std::string variant of BLI_uniquename_cb

Allows to ensure unique name for cases when name is a dynamically
sized string.

Sergey Sharybin requested review from Hans Goudey 2023-10-23 10:03:24 +02:00

Sergey Sharybin requested review from Falk David 2023-10-23 10:03:29 +02:00

Hans Goudey approved these changes 2023-10-23 10:10:35 +02:00

source/blender/blenlib/intern/string_utils.cc

						
				@ -426,1 +429,4 @@

				std::string BLI_uniquename_cb(blender::FunctionRef<bool(blender::StringRef)> unique_check,

				                              const char delim,

				                              const blender::StringRef name)

Hans Goudey commented

2023-10-23 10:10:33 +02:00

There may be an opportunity to avoid a copy by taking a std::string by value as an argument. That way you could call BLI_uniquename_cb(..., ..., std::move(string));

Since this has to store a local copy of the string anyway, that shouldn't be worse in any case.

But maybe that's not worth it right now.

There may be an opportunity to avoid a copy by taking a `std::string` by value as an argument. That way you could call `BLI_uniquename_cb(..., ..., std::move(string));` Since this has to store a local copy of the string anyway, that shouldn't be worse in any case. But maybe that's not worth it right now.

Falk David requested changes 2023-10-23 10:30:22 +02:00

source/blender/blenlib/tests/BLI_string_utils_test.cc Outdated

						
				@ -0,0 +23,4 @@

				  EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', ""), "");

				  EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Baz"), "Baz");

				  EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Foo"), "Foo.001");

				  EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Baz.001"), "Baz.002");

Falk David commented

2023-10-23 10:29:36 +02:00

We should also have a check like EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Bar.003"), "Bar.001"); if current_names has a "Bar.003".
And maybe another that tries adding .999 and returns .1000.

We should also have a check like `EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Bar.003"), "Bar.001");` if `current_names` has a `"Bar.003"`. And maybe another that tries adding `.999` and returns `.1000`.

Iliya Katushenock commented

2023-10-23 14:35:59 +02:00

EXPECT_EQ(BLI_uniquename_cb(random_prefix + to_string(i)), random_prefix + to_string(i + 1));
EXPECT_EQ(BLI_uniquename_cb(random_prefix + to_string(i)), random_prefix + to_string(first_true(all_holded_indices)));
... ?)

`EXPECT_EQ(BLI_uniquename_cb(random_prefix + to_string(i)), random_prefix + to_string(i + 1));` `EXPECT_EQ(BLI_uniquename_cb(random_prefix + to_string(i)), random_prefix + to_string(first_true(all_holded_indices)));` ... ?)

Sergey Sharybin commented

2023-10-24 10:57:28 +02:00

@filedescriptor I've added tests for the Bar.003 and the .999. One thing to note is that EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Bar.003"), "Bar.001"); is not going to pass a test. This variant of unique naming only goes up. This behavior is consistent for current C function and the new C++ function.

@filedescriptor I've added tests for the `Bar.003` and the `.999`. One thing to note is that `EXPECT_EQ(BLI_uniquename_cb(unique_check, '.', "Bar.003"), "Bar.001");` is not going to pass a test. This variant of unique naming only goes up. This behavior is consistent for current C function and the new C++ function.

Sergey Sharybin commented

2023-10-24 10:59:54 +02:00

@mod_moder I am not sure i'll go this route. From the code coverage perspective I think we are all good. From the semantic/logic behavior on user level of this API such automated testing doesn't help much for understanding I think.

Falk David commented

2023-10-24 11:16:56 +02:00

This behavior is consistent for current C function and the new C++ function.

I'm pretty sure that's not true. When I tested it in Blender 3.6, having an object named "Bar.001" and "Bar.003", naming another object "Bar.003" made it "Bar.002".

> This behavior is consistent for current C function and the new C++ function. I'm pretty sure that's not true. When I tested it in Blender 3.6, having an object named "Bar.001" and "Bar.003", naming another object "Bar.003" made it "Bar.002".

Sergey Sharybin commented

2023-10-24 11:24:34 +02:00

See the behavior of the C version of the funciton. It did not change with this PR, and the behavior is consistent when handling the Bar.003.

The ID naming does not use these functions to ensure naming. They use BLI_libblock_ensure_unique_name, which does more comprehensive things like name map caching (for faster addition of objects to a complex files, avoiding N^2 complexity when adding N objects). It also does smarter suffix lookup by trying to reuse minimal available suffix for values below ~1000.

See the behavior of the C version of the funciton. It did not change with this PR, and the behavior is consistent when handling the `Bar.003`. The ID naming does not use these functions to ensure naming. They use `BLI_libblock_ensure_unique_name`, which does more comprehensive things like name map caching (for faster addition of objects to a complex files, avoiding N^2 complexity when adding N objects). It also does smarter suffix lookup by trying to reuse minimal available suffix for values below ~1000.

Falk David commented

2023-10-24 11:31:30 +02:00

I see makes sense.

Sergey Sharybin added 1 commit 2023-10-24 10:55:53 +02:00

87152fd0ec Expand testing of the BLI_uniquename_cb

- Add tests for C version of the function, for behavior comparison.
- Add tests of more cases of naming collisions for C++ version.

Falk David approved these changes 2023-10-24 11:32:02 +02:00

Sergey Sharybin merged commit 7afa5aaa59 into main

2023-10-24 11:36:02 +02:00

Sergey Sharybin referenced this issue from a commit

2023-10-24 11:36:02 +02:00

BLI: Add std::string variant of BLI_uniquename_cb

Sergey Sharybin deleted branch unique_name_stdstring

2023-10-24 11:36:03 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

BLI: Add std::string variant of BLI_uniquename_cb #114052