eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.57k stars 373 forks source link

Clean-up string usage #260

Open marthtz opened 3 years ago

marthtz commented 3 years ago

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.

FerdinandSpitzschnueffler commented 3 years ago

I will work on it

elBoberido commented 3 years ago

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:

The remaining big parts in posh are:

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 commented 3 years ago

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:

The three above could probably be combined, but I don't have a good name.

posh:

dds:

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

elfenpiff commented 3 years ago

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

elBoberido commented 3 years ago

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?

orecham commented 3 years ago

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

elBoberido commented 3 years ago

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

budrus commented 3 years ago

@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.

elBoberido commented 3 years ago

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

elBoberido commented 3 years ago

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