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

2018-02-14 Thread Christophe de Dinechin 


> On 14 Feb 2018, at 15:28, Christophe Fergeau  wrote:
> 
> On Thu, Feb 08, 2018 at 08:16:37AM +0100, Christophe de Dinechin wrote:
>> 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.
>> 
> 
> For maximal clarity, the link that you gave says *not* to use const char
> *, but to use czstring instead... This does not say to "pass it around
> as const char *”.

Sorry if I was not maximally clear ;-)

czstring is just a typedef in the GSL for const char *, but the gsl and 
czstring are not (yet) in the standard. Since we don’t use the gsl, for now we 
have no better than const char *.

PS: Personally, I find it a bit confusing that the Core guidelines refer to 
types that are not in the standard ;-D


Christophe



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-14 Thread Christophe Fergeau 
On Thu, Feb 08, 2018 at 08:16:37AM +0100, Christophe de Dinechin wrote:
> 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.
> 

For maximal clarity, the link that you gave says *not* to use const char
*, but to use czstring instead... This does not say to "pass it around
as const char *".

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2018-02-06 Thread Christophe de Dinechin 


> On 25 Jan 2018, at 11:29, Lukáš Hrázký  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ý 
> ---
> 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 )
> {
> StaticPlugin::InitAll(*this);
> 
> -string pattern = string(directory) + "/*.so";
> +string pattern = directory + "/*.so";
> glob_t globbuf;
> 
> int glob_result = glob(pattern.c_str(), 0, NULL, );
> @@ -77,12 +77,12 @@ void ConcreteAgent::LoadPlugins(const char *directory)
> globfree();
> }
> 
> -void ConcreteAgent::LoadPlugin(const char *plugin_filename)
> +void ConcreteAgent::LoadPlugin(const string _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 );

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 _filename);
> std::vector plugins;
> std::vector 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 , FILE *f_log)
> {
> std::unique_ptr 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

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

2018-01-25 Thread Frediano Ziglio 
> 
> It's a good practice to consistently use std::string in C++ when there
> are no special needs for a char *.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
>  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 )
>  {
>  StaticPlugin::InitAll(*this);
>  
> -string pattern = string(directory) + "/*.so";
> +string pattern = directory + "/*.so";
>  glob_t globbuf;
>  
>  int glob_result = glob(pattern.c_str(), 0, NULL, );
> @@ -77,12 +77,12 @@ void ConcreteAgent::LoadPlugins(const char *directory)
>  globfree();
>  }
>  
> -void ConcreteAgent::LoadPlugin(const char *plugin_filename)
> +void ConcreteAgent::LoadPlugin(const string _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 );
>  // 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 _filename);
>  std::vector plugins;
>  std::vector 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 , FILE *f_log)
>  {
>  std::unique_ptr 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);
>  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";
>  char opt;
>  const char *log_filename = NULL;
>  int logmask = LOG_UPTO(LOG_WARNING);

Acked-by: Frediano Ziglio 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel