[libcxx-commits] [libcxx] [libc++] Optimize __tree::find and __tree::__erase_unique (PR #152370)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> > However for their multi versions this can mean a change in behaviour since it's not longer guaranteed that find will return the first element.
> 
> Should that go in the release notes as well?

Yes, this should probably be release noted. I'll make a patch.

> We have this function that tries to find all bookmark nodes with a given URL:
> 
> ```
> std::vector<BookmarkNode*> BookmarkModel::GetNodesByURL(GURL url) {
>   std::vector<BookmarkNode*> nodes;
>   auto i = nodes_ordered_by_url_set_.find<GURL>(url);
>   while (i != nodes_ordered_by_url_set_.end() && (*i)->url() == url) {
>     nodes.push_back(*i);
>     ++i;
>   }
>   return nodes;
> }
> ```
> 
> `nodes_ordered_by_url_set_` looks like so:
> 
> ```
>   // Used to order BookmarkNodes by URL as well as lookups using GURL.
>   class NodeUrlComparator {
>    public:
>     // Required by std::set to support GURL-based lookups.
>     using is_transparent = void;
> 
>     bool operator()(const BookmarkNode* n1, const BookmarkNode* n2) const {
>       return n1->url() < n2->url();
>     }
>     bool operator()(const BookmarkNode* n1, const GURL& url2) const {
>       return n1->url() < url2;
>     }
>     bool operator()(const GURL& url1, const BookmarkNode* n2) const {
>       return url1 < n2->url();
>     }
>   };
> 
>   using NodesOrderedByUrlSet = std::multiset<BookmarkNode*, NodeUrlComparator>;
>   NodesOrderedByUrlSet nodes_ordered_by_url_set_;
> ```
> 
> The whole thing as a standalone repro (the .sh is meant to run on a mac; needs minor tweaks to run on linux): [repro.zip](https://github.com/user-attachments/files/21971130/repro.zip) This includes a single standalone .cc file that prints `count: 2` without this commit but `count: 1` with it. It also includes a .sh script to compile libc++ and link the included .cc file against the just-built libc++. You probably have a local script for the latter part that you like better :)
> 
> From what I understand, our code is incorrect, and we should use e.g. `equal_range()` instead. I'm only mentioning this here in case other folks run into it.
> 
> (But, see question in the comment above please :))

Yeah, this looks incorrect to me. You want to either use `lower_bound()` to get the first element, or `equal_range()` so you can simply iterate through the equal elements.

https://github.com/llvm/llvm-project/pull/152370