Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: add hash seed to nick colors to allow for reshuffling of colors #635

Closed
wants to merge 3 commits into from

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Dec 25, 2015

This patch adds the possibility of shuffling nick colors without having to choose a different hash algorithm or adding/removing colors in chat_nick_colors. The seed option's string is prepended to the nick before it's hashed. The default of an empty string means that by default, nothing gets changed from previous versions.

Feature requested by m_ben in #weechat.

@flashcode flashcode added the feature New feature request label Dec 25, 2015
@flashcode
Copy link
Member

If you rebase your commit on top of current master, tests should be OK.

@Zarthus
Copy link
Contributor

Zarthus commented Dec 27, 2015

Does exactly what it says on the tin, I've found no problems when I tested this.

@@ -152,6 +152,7 @@ struct t_config_option *config_look_mouse;
struct t_config_option *config_look_mouse_timer_delay;
struct t_config_option *config_look_nick_color_force;
struct t_config_option *config_look_nick_color_hash;
struct t_config_option *config_look_nick_color_hash_seed;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

salt instead of seed would be a more appropriate name: the value is used as a salt to a hash function, not as a seed to some PRNG.

@@ -57,7 +59,21 @@ gui_nick_hash_color (const char *nickname)
if (config_num_nick_colors == 0)
return 0;

ptr_nick = nickname;
length = strlen (CONFIG_STRING(config_look_nick_color_hash_seed)) +
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add conditional to only do the malloc and salt concatenation when the salt is non-empty. Avoids unnecessary allocations and string operations by default and in most cases.

@sim642 sim642 changed the title irc: add hash seed to nick colors to allow for reshuffling of colors core: add hash seed to nick colors to allow for reshuffling of colors Aug 17, 2018
@flashcode flashcode self-assigned this Nov 25, 2019
@flashcode flashcode added the in progress Someone is working on this issue label Nov 25, 2019
@flashcode flashcode added this to the 2.7 milestone Nov 25, 2019
@sim642
Copy link
Member Author

sim642 commented Nov 25, 2019

For future reference, there are further micro-optimizations that could be done but aren't necessary:

  1. Instead of allocating a new string and concatenating with the salt as prefix, it's possible to avoid the allocation. That requires duplicating each hash function implementation to first iterate over the salt characters and then a second loop over the nick characters. This works because all current hash functions work by iterating through the characters once. The downside of this is uglier and duplicate code.

  2. Instead of recalculating the hash function of the salt characters each time, like described above, that value could be cached and reused as the initial value for just iterating over the nick characters. This works because all current hash functions start from a constant value. It would be important to recalculate the cached initial value not only when the seed is changed but also when the hash function is changed.

@flashcode
Copy link
Member

For the first remark, I'll update the function to not allocate a new string, and using a double loop in hash algorithm (no duplicate code), to loop on both strings (if salt is set to a non-empty string).

For the second remark, it could be done, but I think this is not a so critical code, and not necessary to to such optimization.

@flashcode
Copy link
Member

Merged, thanks.

@flashcode flashcode closed this Nov 25, 2019
@flashcode flashcode removed the in progress Someone is working on this issue label Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants