HPCC-31548 Improve qualifyMethodName in HIDL generated code for ESP services #18727
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-31548 Jirabot Action Result: |
There was a problem hiding this comment.
This looks like a good improvement. It seems likely to benefit runtime efficiency (especially in the case you outlined in the ticket) in addition to removing hundreds of lines of code from the hidl-generated output. Just don't forget to take out your commented code before the final push.
|
@ghalliday please merge |
kenrowland
changed the title
tools/hidl/hidlcomp.cpp
Outdated
| @@ -4060,6 +4062,8 @@ void EspServInfo::write_esp_binding_ipp() | |||
| outf("#endif\n"); | |||
| } | |||
|
|
|||
| outs("\tstd::map<std::string, std::string> m_qualifiedMethodNames;\n"); | |||
There was a problem hiding this comment.
This would be better off in CHttpSoapBinding, along with the function qualifyMethodName().
It would avoid duplication, and make the code easier to review - since it would be compiled.
tools/hidl/hidlcomp.cpp
Outdated
| for (mthi=methods; mthi!= nullptr; mthi=mthi->next) | ||
| { | ||
| std::string methodNameKey(mthi->getName()); | ||
| std::transform(methodNameKey.cbegin(), methodNameKey.cend(), methodNameKey.begin(), tolower); |
There was a problem hiding this comment.
A helper function could also be added to make this code cleaner/simpler.
outf("\taddMethod("%s");\n", mthi->getName());
Even better would be
CHttpSoapBinding::registerMethods(std::initializer_list<const char *> methods)
...
init_maps()
{
registerMethods({"a","b","c","d"});
}
|
@ghalliday Had a discussion with Terrance on moving this functionality to the parent class as mentioned above. I wanted to get a point of view from ESDL services. Currently their implementation provides an implementation for the qualify method name, but doesn't do anything. Generally, the implementation optimized by this PR is for HIDL generated services. If this is still something you think we should move, let me know and we'll move it to the parent class. |
So if this was moved to the base class it would be ignored by ESDL services because they override the function? I don't think that is a problem. Or are you saying that the this would override the implementation in a further base class? I think we should be moving the code so it is not being generated lots of times. If necessary you could introduce a new intermediate base class. If you want to talk through the details (particularly if I have missed the main point) please IM me, or arrange a meeting to talk it through. |
|
I will move the functionality into the parent class. |
kenrowland
force-pushed
the
HPCC-31548
branch
2 times, most recently
from
2831bc0 to
fbdd963
Compare
tools/hidl/hidlcomp.cpp
Outdated
| @@ -4060,6 +4059,8 @@ void EspServInfo::write_esp_binding_ipp() | |||
| outf("#endif\n"); | |||
| } | |||
|
|
|||
| outs("\tstd::map<std::string, std::string> m_qualifiedMethodNames;\n"); | |||
There was a problem hiding this comment.
Shouldn't you remove this from here since you have the member in the new CHttpSoapHidlBinding base class?
|
@ghalliday Please merge. As far as size impact, I don't think it is that much. The greater impact is the optimization of qualifying the method name using a map as opposed to sequential if statements until there is a match. |
|
@kenrowland this is failing to build on windows (see the failing test above). |
kenrowland
force-pushed
the
HPCC-31548
branch
2 times, most recently
from
afa32c5 to
f0a43ce
Compare
…ervices Added a map for method name lookup to speed up qualification Signed-Off-By: Kenneth Rowland [email protected]
|
@ghalliday Passes windows build now |
|
@kenrowland there reallly should be a tolower function in jlib that can be called instead of replicating the code. |
|
@ghalliday Based on your previous comment, would you like me to look into adding a tolower function to jlib? |
Added a map for method name lookup to speed up qualification
Signed-Off-By: Kenneth Rowland [email protected]
Type of change:
Checklist:
Smoketest:
Testing: