Remove deprecated score::StringLiteral#745
Conversation
d50e760 to
263c6ca
Compare
limdor
left a comment
There was a problem hiding this comment.
const char * has multiple problems and I do not think this should be the substitute for StringLiteral.
- An
std::stringcan be converted implicitly fromconst char *ending with dynamic memory allocation const char*has the problem that it could be a pointer to characters or a pointer to an array of bytes.charmeaning is way too overloaded in the standard- The fact that it is a pointer, it can end up in all the issues with pointer decay
- It has no notion of being null terminated or not
Because of that, my proposal would be to go for better alternatives.
- For main arguments, go with a container of null terminated string views. We can use https://github.com/eclipse-score/baselibs/tree/main/score/language/safecpp/string_view and https://github.com/eclipse-score/baselibs/blob/main/score/string_manipulation/arguments/arguments.h for that.
- For global constant string literals with internal linkage, we can use zstring_view https://github.com/eclipse-score/baselibs/tree/main/score/language/safecpp/string_view
- For the rest, I would propose to take a look one by one, depending if they are owning or not, and depending if they are null terminated or not.
Additional note: I saw that in several places you removed the usage of StringLiteral, but the dependency to the target and the include is still there. If we remove StringLiteral we should remove it completly and also remove the include files. Of course it can happen that we break some code if someone relies on the transitive dependency but imho this is a price the users need to pay if they did not have proper includes.
| constexpr auto kDeprecatedConfigurationPathCommandLineKey = std::string_view{"-service_instance_manifest"}; | ||
| constexpr auto kConfigurationPathCommandLineKey = std::string_view{"--service_instance_manifest"}; | ||
|
|
||
| void VerifyNullTermination(const std::int32_t argc, const char* argv[]) |
There was a problem hiding this comment.
This is something that you cannot do. The problem of this is that if it is null terminated, you will find it, but if it is not null terminated, you hit undefined behavior.
In the second case, it could even be that you hit some null character in memory that it is totally unrelated.
LittleHuba
left a comment
There was a problem hiding this comment.
Please use C++ infrastructure. E.g. zstring_view or Arguments helper in baselibs.
|
OK. I guess, I will close this PR then. The thing is: All the usage of @limdor , @LittleHuba : Maybe I'm wrong, but to me it seems, that all your input means/can only solved via changng the PUBLIC interface. I.e. require the user NOT to hand over the |
|
Yes, it means breaking public API. But the first step would be to provide a second overload that allows the user to use a container of zstring_view and deprecate the API that uses |
263c6ca to
9476fed
Compare
88bce04 to
8065b78
Compare
Removed usage of deprecated score::StringLiteral in Runtime. Replaced with const safecpp::zstring_view on implementation level. Public/user facing APIs using StringLiteral or char* have been marked deprecated and overloads taking safecpp::zstring_view have been introduced.
8065b78 to
ee86368
Compare
Removed usage of deprecated
score::StringLiteral in Runtime.
Replaced with const char* and
added verification for NULL-termination.