Cache functions by name. #11470
Cache functions by name. #11470
Conversation
libsolidity/ast/AST.h
Outdated
| std::multimap<std::string, FunctionDefinition const*> const& functions = m_definedFunctionsByName.init([&]{ | ||
| std::multimap<std::string, FunctionDefinition const*> result; | ||
| for (FunctionDefinition const* fun: filteredNodes<FunctionDefinition>(m_subNodes)) | ||
| result.insert({fun->name(), fun}); |
There was a problem hiding this comment.
FunctionDefinition is only forward-declared here.
chriseth
force-pushed
the
cacheVirtualLookup
branch
2 times, most recently
from
45a7d83 to
4bc76cb
Compare
| auto definedFunctions(std::string const& _name) const { | ||
| auto&& [b, e] = definedFunctionsByName().equal_range(_name); | ||
| return ranges::span<decltype(*b)>(b, e) | ranges::views::transform([](auto const& _item) { return _item.second; }); | ||
| } |
There was a problem hiding this comment.
That's what we wrote about in the chat earlier :-).
I think it has to be
return ranges::subrange(b, e) | ranges::views::values;
to work.
libsolidity/ast/AST.h
Outdated
| std::vector<ModifierDefinition const*> functionModifiers() const { return filteredNodes<ModifierDefinition>(m_subNodes); } | ||
| std::vector<FunctionDefinition const*> definedFunctions() const { return filteredNodes<FunctionDefinition>(m_subNodes); } | ||
| auto definedFunctions() const { | ||
| return definedFunctionsByName() | ranges::views::transform([](auto const& _item) { return _item.second; }); |
There was a problem hiding this comment.
| return definedFunctionsByName() | ranges::views::transform([](auto const& _item) { return _item.second; }); | |
| return definedFunctionsByName() | ranges::views::values; |
does that work?
There was a problem hiding this comment.
Yes, that should work, when including <range/v3/view/map.hpp> and is nicer.
There was a problem hiding this comment.
I tried it, but it doesn't - it's a multimap - d'oh!
There was a problem hiding this comment.
No really, I'm underwhelmed by the power of that library.
There was a problem hiding this comment.
It does work for me for a multimap :-).
There was a problem hiding this comment.
At least this:
#include <range/v3/all.hpp>
#include <iostream>
#include <map>
int main()
{
std::multimap<int, int> x{{1,1},{2,3},{2,4}};
auto r = x | ranges::views::values;
for (auto v : r)
std::cout << v << std::endl;
std::cout << std::endl;
auto&& [b,e] = x.equal_range(2);
auto r2 = ranges::subrange(b,e) | ranges::views::values;
for (auto v : r2)
std::cout << v << std::endl;
}
compiles just fine for me.
chriseth
force-pushed
the
cacheVirtualLookup
branch
3 times, most recently
from
2bb0ae0 to
165d540
Compare
|
Tests fail because the order of iteration is different. I think it would be good to preserve it - I'll think about it. |
ekpyron
left a comment
ekpyron
left a comment
There was a problem hiding this comment.
Apart from the iteration order question, this looks good to me apart from superfluous headers.
libsolidity/ast/AST.cpp
Outdated
| #include <range/v3/view/map.hpp> | ||
| #include <range/v3/span.hpp> | ||
|
|
There was a problem hiding this comment.
Neither are needed here anymore, are they?
libsolidity/ast/AST.h
Outdated
|
|
||
| #include <range/v3/view/subrange.hpp> | ||
| #include <range/v3/view/map.hpp> | ||
| #include <range/v3/view/transform.hpp> |
There was a problem hiding this comment.
transform isn't needed anymore, is it?
|
Regarding the order: Since C++11 the standard dictates And I don't think there will be a way to have efficient lookup by name without sorting by name. |
chriseth
force-pushed
the
cacheVirtualLookup
branch
2 times, most recently
from
1aaaae8 to
b9d4158
Compare
chriseth
force-pushed
the
cacheVirtualLookup
branch
from
b9d4158 to
25a28f9
Compare
ekpyron
left a comment
ekpyron
left a comment
There was a problem hiding this comment.
Needs rebase, apart from that still approved.
chriseth
force-pushed
the
cacheVirtualLookup
branch
from
25a28f9 to
0d948eb
Compare
No description provided.