RSDK-9299 - refactor get_resource_name to be a non-proto method #329
Conversation
| // https://bugs.llvm.org/show_bug.cgi?id=41141 | ||
| // | ||
| // NOLINTNEXTLINE | ||
| boost::split(name_parts, long_name, boost::is_any_of(":")); |
There was a problem hiding this comment.
q: Why do we explicitly use fully qualified names like boost::split or std::something instead of bringing them into scope with using or using namespace in the code?
There was a problem hiding this comment.
good question! I tend to like fully qualified names when they're not overly verbose. I appreciate the explicitness. And, a broader using line can add more than we need to scope and create potential for namespace collision.
Also in this case, boost::split and boost::is_any_of are only used once each in this file so using them would actually make the code (slightly) more verbose, not less.
| @@ -61,6 +61,10 @@ class ClientContext { | |||
| grpc::ClientContext wrapped_context_; | |||
| }; | |||
|
|
|||
| /// @brief Given a fully qualified resource name, returns remote name (or "" if no remote name | |||
There was a problem hiding this comment.
what is a remote name and short name? and what purpose do they server as opposed to resource names?
There was a problem hiding this comment.
a remote name is the name of a remote that the resource is part of, and the short name is just the name of the resource. so we could have remote1:my_arm as a long name, which is comprised of a remote name (remote1) and a short name my_arm).
| // https://bugs.llvm.org/show_bug.cgi?id=41141 | ||
| // | ||
| // NOLINTNEXTLINE | ||
| boost::split(name_parts, long_name, boost::is_any_of(":")); |
There was a problem hiding this comment.
is it ever possible that long_name does not contain colons? in this case remote_name will be empty and name will be the entire long_name (right?) I am not sure if this is an edge case we need to worry about but if it is, is this behavior okay?
There was a problem hiding this comment.
I don't think this is possible. A long name is of form {remote}:{short_name} (or {remote1}:...{remoteN}:{short_name}. If there's no colon then we just have a short name so we just return the name as-is, which is ideal.
| *r.mutable_type() = kComponent; | ||
| return r; | ||
| Name Component::get_resource_name() const { | ||
| return Resource::get_resource_name(kComponent); |
There was a problem hiding this comment.
how does this function know what kComponent is? I cant seem to find it as a global variable anywhere...?
There was a problem hiding this comment.
kComponent and kService are defined in utils.hpp, which is included in this file (and in service.cpp below).
| *r.mutable_type() = kService; | ||
| return r; | ||
| Name Service::get_resource_name() const { | ||
| return Resource::get_resource_name(kService); |
There was a problem hiding this comment.
same as kComponent but where is kService coming from? how does the function know it exists
good questions! no new tests but there are existing proto conversion tests. The reasoning is that proto types affect the ABI but we don't have control their shape, so we want to remove them from the user API as much as possible. Also, a |
| boost::split(name_parts, long_name, boost::is_any_of(":")); | ||
| auto name = name_parts.back(); | ||
| name_parts.pop_back(); | ||
| auto remote_name = name_parts.empty() |
There was a problem hiding this comment.
Maybe we should be checking name_parts.empty() above before calling back(), that's UB if the container is empty and I think since this is in a public header we might want to be slightly more cautious about inputs
Also could be good to add a unit test or two
There was a problem hiding this comment.
Oh wait just saw your reply to Julie above, so split will return a vector with one element if there's no separator?
There was a problem hiding this comment.
Oh wait just saw your reply to Julie above, so split will return a vector with one element if there's no separator?
Yep! I've just added a test to highlight a couple cases just to be be explicit.
No description provided.