iceoryx
iceoryx copied to clipboard
Clean-up string usage
Brief feature description
Iceoryx uses a variety of strings (std::string
, cxx::string<>
, cxx::CString100
, etc).
Once #253 is merged this mishmash should be cleaned-up.
Detailed information
Only use cxx:string<>
or types created from it.
-
[x] No std::string in public API
-
[ ] No more std::strings in code base
- [ ] iceoryx_hoofs
- [x] Taken from: #1196 Refactor
cxx::convert
so that allcxx::string
capacities are handled
- [x] Taken from: #1196 Refactor
- [ ] iceoryx_posh
- [x] ~~iceoryx_dds~~
- [ ] iceoryx_hoofs
-
[x] No more CSring100 in code base
-
[ ] Common convention for string aliases. Currently we have:
- alias without
_t
, e.g.IdString
- alias with
_t
, e.g.ProcessName_t
- alias nested in a class, e.g.
PosixUser::string_t
- alias with a constexpr as string length, e.g.
cxx::string<MAX_LENGTH>
- alias without a constexpr as string length, e.g.
cxx::string<100>
- see also https://github.com/eclipse/iceoryx/pull/373/files#r525922015 and https://github.com/eclipse/iceoryx/pull/373/files#r525922656
- [ ] cleanup iceoryx_hoofs
- [x] cleanup iceoryx_posh
- [x] cleanup iceoryx_dds
- alias without
I will work on it
once #383 and #379 are merged, I've a branch ready for review which essentially removes the std::string
from public API in iceoryx_posh. It's still indirectly used by e.g. cxx::Serialization
but that's only internally used.
The remaining big parts in utils are:
- cxx::Convert
- cxx::Serialization
- posix::MessageQueue
- posix::UnixDomainSocket
- the logger
The remaining big parts in posh are:
- MqInterface and MqMessage due to essentially all the remaining parts of utils
- PoshRuntime due to MqMessage and cxx::Serialization
I'd suggest to tackle this together with or after #332. There is a proposal how to refactor the IPC communication which by itself would remove some of the std::string
Regarding the topic for a convention for string aliases. I would suggest to have a _t
suffix. I would also suggest to have public aliases in iceoryx_posh_types.hpp. I'm not sure if we should do the same in utils, but probably we should.
Aliases which are only used private should remain in the class, since that's an implementation detail.
utils:
- posix::AccessControl::string_t -> can contain a posix user or group name
- posix::PosixUser::string_t
- posix::PosixGroup::string_t
The three above could probably be combined, but I don't have a good name.
posh:
- capro::IdString
- mepoo::SegmentManager::SegmentMapping::string_t -> used for shared memory name, which currently is either "/iceoryx_mgmt" or posix group with a leading slash
- iox::ConfigFilePathString_t
- iox::ProcessName_t
- iox::RunnableName_t
- roudi::ShmNameString -> could be combined with SegmentMapping::string_t and should probably be in utils
- version::BuildDateStringType
- version::CommitIdStringType
- version::SerializationStringType -> only used in version_info.cpp
dds:
- dds::IdString -> in data_reader.hpp, data_writer.hpp and dds_types.hpp; I assume the first two could be removed
I would wait with this until #360 is merged since this changes would create massive merge conflicts for no good reason.
To prevent too much bike-shedding, I would suggest to discuss only the convention for the name of the alias and where they should live, not if the length should be a constexpr. We can do that afterwards.
@ithier, @marthtz, @elfenpiff, @MatthiasKillat you were the ones who had the most remarks for this, please comment
@elBoberido I like your suggestion and I do support it completely!
utils:
* posix::AccessControl::string_t -> can contain a posix user or group name * posix::PosixUser::string_t * posix::PosixGroup::string_t
The three above could probably be combined, but I don't have a good name.
How about PosixAccessorName_t
for all of the three?
I'm a bit confused, are we discussing a general convention for all fixed string aliases, or what to call each individual alias ?
I'm a bit confused, are we discussing a general convention for all fixed string aliases, or what to call each individual alias ?
A general convention for fixed string aliases and additionally, since some of them could be unified, I'd appreciate some suggestions.
The proposal was to add a _t
suffix. So e.g. version::BuildDateStringType
would become version::BuildDateString_t
.
I'd also suggest to have either Name
or String
in the alias, so roudi::ShmNameString
would become roudi::ShmName_t
@elBoberido. Did we already reached the point we wanted to have for 1.0? I think we agreed to not make the whole list now but only the public API related stuff. If this is true and the check box list in the description is up to date, I would remove this issue from 1.0 scope.
@budrus the harmonization of the string aliases are missing. Once that is done, we are probably good for 1.0
@budrus from my point of view, the remaining parts can be done after the 1.0 release