> On 6 Feb 2018, at 13:23, Lukáš Hrázký <lhra...@redhat.com> wrote: > > On Tue, 2018-02-06 at 12:02 +0100, Christophe de Dinechin wrote: >>> On 25 Jan 2018, at 11:29, Lukáš Hrázký <lhra...@redhat.com> wrote: >>> >>> It's a good practice to consistently use std::string in C++ when there >>> are no special needs for a char *. >> >> OK. Catching up on past e-mail here. FWIW, I disagree with the rationale and >> therefore the change. std::string represents dynamic string. There is no >> reason to use std::string if all you have are text constants. It makes the >> code more verbose and less efficient with no real benefit (see below >> examples in your case). >> >> I really don’t mind much on this specific case, in particular because there >> are actual string computations involved, so it half makes sense in that >> specific case. But let’s not get into the habit of quoting “good practice” >> for bad reasons, i.e. giving the impression that we don’t understand the >> rationale for the good practice being involved. > > Let me rephrase then: I consider it a good practice :) Sorry for > generalizing. > > First, the dynamic allocation/destructor overhead is a premature > optimization beyond critical paths in the code (which fall into the > 'special needs' category mentioned above), so maintainability takes > precedence. > > In this case, maintainability is not an issue either. But in many cases > std::string is a higher level construct that improves maintainability. > And I think it's good to have a consistent usage of std::string, > instead of making a case by case judgement every time and having both > mixed up in the code depending on the author's feeling at the time... > (and then sometimes having to go through code and replace char* with > std::string when the need comes - or leave the code inconsistent) > > That's coming from the C++ side of things. I understand you may have a > different opinion coming from the other side :)
Coming back on-list after an off-list digression discussing this topic with Lukas… Using std::string by default is *not* considered a good practice in C++. The reference for C++ good practices are probably best summarized by the C++ Core Guidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md. Regarding strings, see https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rstr-zstring, which specifically states this: Don't convert a C-style string to string unless there is a reason to. “abc” is a C-style string, even in C++. You should pass it around as const char *, aka czstring as long as you can. *That* is the C++ good practice AFAIK. As an aside, since we are starting to write C++ code, I believe we should add a reference to the C++ Core Guidelines to our style guide, and add it as a bookmark to our browsers. Thanks, Christophe > > Lukas > >>> >>> Signed-off-by: Lukáš Hrázký <lhra...@redhat.com> >>> --- >>> src/concrete-agent.cpp | 10 +++++----- >>> src/concrete-agent.hpp | 4 ++-- >>> src/spice-streaming-agent.cpp | 6 +++--- >>> 3 files changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp >>> index 9f1295a..b7d4bfe 100644 >>> --- a/src/concrete-agent.cpp >>> +++ b/src/concrete-agent.cpp >>> @@ -56,11 +56,11 @@ void ConcreteAgent::AddOption(const char *name, const >>> char *value) >>> options.insert(--options.end(), ConcreteConfigureOption(name, value)); >>> } >>> >>> -void ConcreteAgent::LoadPlugins(const char *directory) >>> +void ConcreteAgent::LoadPlugins(const string &directory) >>> { >>> StaticPlugin::InitAll(*this); >>> >>> - string pattern = string(directory) + "/*.so"; >>> + string pattern = directory + "/*.so"; >>> glob_t globbuf; >>> >>> int glob_result = glob(pattern.c_str(), 0, NULL, &globbuf); >>> @@ -77,12 +77,12 @@ void ConcreteAgent::LoadPlugins(const char *directory) >>> globfree(&globbuf); >>> } >>> >>> -void ConcreteAgent::LoadPlugin(const char *plugin_filename) >>> +void ConcreteAgent::LoadPlugin(const string &plugin_filename) >>> { >>> - void *dl = dlopen(plugin_filename, RTLD_LOCAL|RTLD_NOW); >>> + void *dl = dlopen(plugin_filename.c_str(), RTLD_LOCAL|RTLD_NOW); >>> if (!dl) { >>> syslog(LOG_ERR, "error loading plugin %s: %s", >>> - plugin_filename, dlerror()); >>> + plugin_filename.c_str(), dlerror()); >>> return; >>> } >>> >>> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp >>> index 828368b..2449cb3 100644 >>> --- a/src/concrete-agent.hpp >>> +++ b/src/concrete-agent.hpp >>> @@ -30,13 +30,13 @@ public: >>> } >>> void Register(Plugin& plugin) override; >>> const ConfigureOption* Options() const override; >>> - void LoadPlugins(const char *directory); >>> + void LoadPlugins(const std::string &directory); >> >> All use cases are text constants, so not necessary to switch to string. >> >>> // pointer must remain valid >>> void AddOption(const char *name, const char *value); >>> FrameCapture *GetBestFrameCapture(); >>> bool PluginVersionIsCompatible(unsigned pluginVersion) const override; >>> private: >>> - void LoadPlugin(const char *plugin_filename); >>> + void LoadPlugin(const std::string &plugin_filename); >>> std::vector<std::shared_ptr<Plugin>> plugins; >>> std::vector<ConcreteConfigureOption> options; >>> }; >>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp >>> index f36921d..87e8fa3 100644 >>> --- a/src/spice-streaming-agent.cpp >>> +++ b/src/spice-streaming-agent.cpp >>> @@ -347,13 +347,13 @@ static void cursor_changes(Display *display, int >>> event_base) >>> } >>> >>> static void >>> -do_capture(const char *streamport, FILE *f_log) >>> +do_capture(const string &streamport, FILE *f_log) >>> { >>> std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture()); >>> if (!capture) >>> throw std::runtime_error("cannot find a suitable capture system"); >>> >>> - streamfd = open(streamport, O_RDWR); >>> + streamfd = open(streamport.c_str(), O_RDWR); >> >> Example where it makes the text more verbose. >> >>> if (streamfd < 0) >>> // TODO was syslog(LOG_ERR, "Failed to open %s: %s\n", streamport, >>> strerror(errno)); >>> throw std::runtime_error("failed to open streaming device"); >>> @@ -433,7 +433,7 @@ done: >>> >>> int main(int argc, char* argv[]) >>> { >>> - const char *streamport = "/dev/virtio-ports/com.redhat.stream.0"; >>> + string streamport = "/dev/virtio-ports/com.redhat.stream.0”; >> >> This dynamically allocates memory and inserts a destructor call at the end >> of the function, whereas before it was passing a static pointer. >> >>> char opt; >>> const char *log_filename = NULL; >>> int logmask = LOG_UPTO(LOG_WARNING); >>> -- >>> 2.15.1 >>> >>> _______________________________________________ >>> Spice-devel mailing list >>> Spice-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/spice-devel >> >> > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/spice-devel _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
