[Spice-devel] [PATCH spice-streaming-agent v2 3/4] use std::string where it's easy to replace

[Christophe de Dinechin]

> On 25 Jan 2018, at 11:29, Lukáš Hrázký <lhrazky at 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.


> 
> Signed-off-by: Lukáš Hrázký <lhrazky at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel