On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
<y.karadz@xxxxxxxxx> wrote:
>
>
>
> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
> > On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> wrote:
> >>
> >> const char *lib = plugin.toStdString().c_str();
> >>
> >> This line is a bad idea because the returned array may (will) be
> >> invalidated when the destructor of std::string is called.
> >>
> >> Signed-off-by: Yordan Karadzhov <ykaradzhov@xxxxxxxxxx>
> >> ---
> >> kernel-shark/src/KsUtils.cpp | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
> >> index 34b2e2d..d7b1753 100644
> >> --- a/kernel-shark/src/KsUtils.cpp
> >> +++ b/kernel-shark/src/KsUtils.cpp
> >> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
> >>
> >> auto lamRegUser = [&kshark_ctx](const QString &plugin)
> >> {
> >> - const char *lib = plugin.toStdString().c_str();
> >> + const char *lib = plugin.toLocal8Bit().data();
> >> kshark_register_plugin(kshark_ctx, lib);
> >> };
> >>
> >> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
> >>
> >> auto lamUregUser = [&kshark_ctx](const QString &plugin)
> >> {
> >> - const char *lib = plugin.toStdString().c_str();
> >> + const char *lib = plugin.toLocal8Bit().data();
> >> kshark_unregister_plugin(kshark_ctx, lib);
> >> };
> >
> > Doesn't the new version have the same problem with the temporary QByteArray?
> >
> > How about saving the data in a local std::string/QByteArray variable?
> >
> > std::string lib = plugin.toStdString();
> > kshark_register_plugin(kshark_ctx, lib.c_str());
> >
>
> Hi Slavi,
>
> As far I can understand toStdString() will create a fresh std::string
> and this string will has its own copy of the characters. However, this
> copy gets out-of-scope as soon as we hit the semicolon of the line.
>
> My understanding was that toLocal8Bit().data() makes no copy so the
> array will go out-of-scope only when the QString is destroyed.
I did some digging into QString's implementation. From my reading of
the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which
does allocation, copying/transform-to-latin1 and returns the
QByteArray. So it seems that toLocal8Bit() is still making a copy.
[1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275
Cheers,
-- Slavi
>
> But anyway, your solution looks cleaner and safer.
> I will send another version of the patch.
> Thanks a lot!
> Y.
>
>
> > Thanks!
> >
> >
> > --
> > Slavomir Kaslev
> >
--
Slavomir Kaslev
|