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

[Christophe de Dinechin]

> On 6 Feb 2018, at 13:23, Lukáš Hrázký <lhrazky at 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ý <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.
> 
> 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ý <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
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel