Individualize NameHashSet Hashing & Revisit #709 #740
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I wonder if we should just change the hash strategy for the |
|
That’s probably a cleaner way of accomplishing this. I’ll submit a patch in the near future |
Basically, in order to implement our own (actual) hash policy in `PluginSys.h`, we needed to remove the blanket implementation of `hash` that was used before. Now, each policy must implement `hash` along with `matches` in order to be used with `NameHashSet`. While this does force us to change every implementation of policies across the entirety of sourcemod, it allows core to use flexible implementations of `hash`.
Headline
changed the title
|
@asherkin can you add this to your list for a review? duck says he's cool to take this into 1.10 and if all is well after a while it's cool to cherrypick into 1.9 |
asherkin
approved these changes
Jul 10, 2018
There was a problem hiding this comment.
I'm perfectly happy with this change - the only thing I was waiting on was having a shot at using SFINAE to avoid implement hash everywhere, but lets ship this.
(I can't merge things sanely atm, which also applies to your other PRs I have r+'d but not merged myself.)
Headline
added a commit
that referenced
this pull request
Jul 28, 2018
* Make mac/win lookups lowercase'd * Revert #709 & 81042cc * Adjust HashPolicy implementation across sourcemod Basically, in order to implement our own (actual) hash policy in `PluginSys.h`, we needed to remove the blanket implementation of `hash` that was used before. Now, each policy must implement `hash` along with `matches` in order to be used with `NameHashSet`. While this does force us to change every implementation of policies across the entirety of sourcemod, it allows core to use flexible implementations of `hash`. * Remove logic duplication * Improve lowercase checks
Headline
added a commit
that referenced
this pull request
Jul 28, 2018
asherkin
pushed a commit
that referenced
this pull request
Aug 11, 2018
This is a clone of #740, but without the amtl ke::AString lowercase which was implemented in a new version of amtl that 1.9-dev isn't pinned to. Updating this pin and moving fixes is beyond what should go in 1.9, and this fixes a annoying and user-impactful bug with reload/unloading plugins on windows. Currently in 1.9, once a plugin is loaded into the pluginsys, they must be used with lowercase characters *only*, since pr #709 ignorantly modified their names. ``` // test.smx exists in /plugins/ sm plugins load TEST.smx // successful sm plugins unload TEST.smx // TEST.smx not found, it's actually test.smx ``` This pr fixes that error by converting *all* lookups, not just loads.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On #709, we forgot to convert the
NameHashSet<CPlugin *>'s lookups to be lowercase. This will fix some casing issues people were experiencing trying to unload their (case-edited) plugins.EDIT (12/27/17):
Upon asherkin’s message below, I reverted the first commit of this PR & the #709 commit. The new solution to this entire problem, including the one #709 tried to solve, is to change the way
NameHashSethashes the key by forcing policies to also implement ahashmethod which will be used instead of the blanket implementation ofhashthat existed before.Edit (6/17/2018):
Fixes #831