iceoryx icon indicating copy to clipboard operation
iceoryx copied to clipboard

Clean-up string usage

Open marthtz opened this issue 3 years ago • 10 comments

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 all cxx::string capacities are handled
    • [ ] iceoryx_posh
    • [x] ~~iceoryx_dds~~
  • [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

marthtz avatar Aug 26 '20 11:08 marthtz

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

elBoberido avatar Nov 20 '20 15:11 elBoberido

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 avatar Nov 24 '20 11:11 elBoberido

@elBoberido I like your suggestion and I do support it completely!

elfenpiff avatar Nov 24 '20 12:11 elfenpiff

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?

elBoberido avatar Nov 24 '20 14:11 elBoberido

I'm a bit confused, are we discussing a general convention for all fixed string aliases, or what to call each individual alias ?

orecham avatar Nov 24 '20 16:11 orecham

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 avatar Nov 24 '20 16:11 elBoberido

@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 avatar Nov 30 '20 14:11 budrus

@budrus the harmonization of the string aliases are missing. Once that is done, we are probably good for 1.0

elBoberido avatar Nov 30 '20 17:11 elBoberido

@budrus from my point of view, the remaining parts can be done after the 1.0 release

elBoberido avatar Dec 16 '20 13:12 elBoberido