Use Standard unordered_multimap
#111
Conversation
In ApiReader.cpp, ApiReader::clearAll() is iterating through apiList, which is multi. In ApiReader.h, ApiReader::getScoredApi() should be declared as taking multi to match its definition in ApiReader.cpp, as it's taking iterators into apiList. This was compiling despite the single/multi mismatch because in MSVC's implementation, for both legacy hash_map and Standard unordered_map, the containers have the same iterator types regardless of single/multi, but it's still a conformance issue and should be fixed.
Summary of ChangesHello @StephanTLavavej, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project to use standard C++ containers ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that modernizes the codebase by replacing the deprecated stdext::hash_multimap with the standard std::unordered_multimap. The changes are clean, and the fix for the iterator type mismatch is a great catch. I have a few minor suggestions to further improve the readability and maintainability of the updated code.
| { | ||
| public: | ||
| static stdext::hash_multimap<DWORD_PTR, ApiInfo *> apiList; //api look up table | ||
| static std::unordered_multimap<DWORD_PTR, ApiInfo *> apiList; //api look up table |
There was a problem hiding this comment.
To improve readability and maintainability, consider introducing a type alias for std::unordered_multimap<DWORD_PTR, ApiInfo *>. This makes the code less verbose and easier to change in the future if the container type needs to be updated again.
This alias can then be used throughout ApiReader.h and ApiReader.cpp where this long type is referenced (e.g., line 73 in this file, and lines 15, 693, 776, 1119 in ApiReader.cpp).
using ApiList = std::unordered_multimap<DWORD_PTR, ApiInfo *>;
static ApiList apiList; //api look up tableThere was a problem hiding this comment.
This would be a reasonable improvement, but I intentionally refrained from performing unrelated cleanups, beyond fixing the single/multi confusion.
| ApiInfo * ApiReader::getApiByVirtualAddress(DWORD_PTR virtualAddress, bool * isSuspect) | ||
| { | ||
| stdext::hash_multimap<DWORD_PTR, ApiInfo *>::iterator it1, it2; | ||
| std::unordered_multimap<DWORD_PTR, ApiInfo *>::iterator it1, it2; |
There was a problem hiding this comment.
The iterator it2 is declared but never used within the getApiByVirtualAddress function. It should be removed to improve code clarity and avoid potential confusion.
std::unordered_multimap<DWORD_PTR, ApiInfo *>::iterator it1;There was a problem hiding this comment.
I confirm that it2 is unused and should be removed (good bot, I didn't notice that). However, I am intentionally refraining from performing unrelated cleanups in this PR.
| for ( std::unordered_multimap<DWORD_PTR, ApiInfo *>::iterator it = apiList.begin(); it != apiList.end(); ++it ) | ||
| { | ||
| delete it->second; | ||
| } |
There was a problem hiding this comment.
This traditional for loop can be modernized by using a C++11 range-based for loop. It's more concise and less error-prone.
for (const auto& pair : apiList)
{
delete pair.second;
}There was a problem hiding this comment.
I agree that this would be an improvement and was tempted to perform it, but intentionally refrained from unrelated cleanups in this PR (I even avoided using auto for the iterator type, since there may be a coding convention that I am unaware of).
|
Hi Stephan, thanks for bringing this issue to my attention. I feel honoured that my beloved capemon would benefit from input from yourself and your team, but I probably don't need to point out that this project owes a lot to its heritage as I am sadly not skilled enough to have written all the code here! The code in question is originally from Scylla by @NtQuery: https://github.com/NtQuery/Scylla which looks abandoned for more or less a decade. This incarnation may be the only survivor, and I am open to improving it wherever possible but lack skills when it comes to C++. For that reason I will of course gratefully accept your changes but would equally be happy to implement any other improvements you can see! |
|
Thank you! I have my hands full with being the only maintainer of MSVC's STL reviewing contributor PRs so I don't have other improvements to suggest (other than using range- |
|
I have now implemented all of Gemini's suggestions. Thanks again! |
2 participants
I'm the primary maintainer of Microsoft's C++ Standard Library implementation. The MSVC developers regularly build popular open-source projects, including yours, with development versions of the build tools in order to find and fix regressions in the compiler and libraries before they're released to the world. This also allows us to provide advance notice of breaking changes, which is the case here.
I'm working on removing our non-Standard
<hash_map>implementation, which has been deprecated for many years, with microsoft/STL#5764. Your project has been defining the escape hatch macro which warned that "<hash_map>is deprecated and will be REMOVED." Fortunately, switching your code over to use the Standard container was simple enough for me to do it (even though I know very little about your codebase) and I was even able to fix a small mistake with iterator types.Although we're building your project with VS 2026 and your README says that you're using VS 2017, these changes will be compatible with VS 2017, and will unblock future upgrades for you.
Hope this helps!
Commits
<unordered_map>instead of<hash_map>.std::unordered_multimapinstead ofstdext::hash_multimap.unordered_multimap::iteratorinstead ofhash_map::iterator.ApiReader::clearAll()is iterating throughapiList, which is multi.ApiReader::getScoredApi()should be declared as taking multi to match its definition in ApiReader.cpp, as it's taking iterators intoapiList.hash_mapand Standardunordered_map, the containers have the same iterator types regardless of single/multi, but it's still a conformance issue and should be fixed._SILENCE_STDEXT_HASH_DEPRECATION_WARNINGS.