fix: stardict qstring modification

[xiaoyifang]

the stardict qstring part has caused crash sometimes.
Fix #1702

basic rule: avoid unnecessary conversions between QString and string

Changes:
1.

QString newTag   = QString::fromUtf8(
            ( addAudioLink( href, getId() ) + "<span class=\"sdict_h_wav\"><a href=" + href + ">" ).c_str() );

to
string only.

2. return (articleText.toUtf8.data());
   to

     auto text = articleText.toUtf8();
      return text.data();

QString stripHtml( const QString & tmp )
to
QString stripHtml( QString tmp )

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[shenlebantongying]

The difference is pretty subtle.

• in the new code, std::string(toUtf8().data()) will seek for the null byte again
• in the old code, toStdString's std::string(toUtf8().data(),toUtf8().size()) will use the articleText's actual length

Maybe there should be a comment that clarify articleText may contain null byte, and we want to cut off earlier?

[xiaoyifang]

the original code is
return (articleText.toUtf8().data()); I changed it into return articleText.toStdString() ,crash still reported on this line.
So I split it into two lines.
I guess articleText.toUtf8() will generate a temporary string . when chained with .data() . sometimes the temporary variable has no longer existed. and a crash will throw.

From real environment test ,the crash has more probability to happen when in multithread cases.

[xiaoyifang]

• in the old code, toStdString's std::string(toUtf8().data(),toUtf8().size()) will use the articleText's actual length

the old code is return (articleText.toUtf8.data()); ,toStdString() is my first attempt.

[xiaoyifang]

https://bugreports.qt.io/browse/QTBUG-63274

[xiaoyifang]

Any relation?

https://doc.qt.io/qt-6/qstring.html#details:~:text=Note%3A%20Due,for%20more%20information.

[xiaoyifang]

I also can not be sure.

But Qt+MSVC +QString are more likely to have issue as the https://bugreports.qt.io/browse/QTBUG-63274 implies

Maybe some link option in CMAKE , /MD /MT etc?

[shenlebantongying]

This seems to be the correct way to set /MD /MT

set_property(TARGET foo PROPERTY
  MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL")

https://github.com/qt/qtbase/blob/02cb165ef8050230b477358e4136e9f0acd83eb6/cmake/QtPublicTargetHelpers.cmake#L395-L397

However, CMake's doc says

If the property is not set, then CMake uses the default value MultiThreaded$<$CONFIG:Debug:Debug>DLL to select a MSVC runtime library.

So, I suppose it is set correctly by CMake already.

https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html

[xiaoyifang]

should cmake_policy be set?

https://cmake.org/cmake/help/latest/prop_tgt/MSVC_RUNTIME_LIBRARY.html#:~:text=Note%20This%20property%20has%20effect%20only%20when%20policy%20CMP0091%20is%20set%20to%20NEW%20prior%20to%20the%20first%20project()%20or%20enable_language()%20command%20that%20enables%20a%20language%20using%20a%20compiler%20targeting%20the%20MSVC%20ABI.

[shenlebantongying]

No,cmake_minimum_required will set policy version, and we currently set it aggressively high 😅.

https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html#policy-settings

goldendict-ng/CMakeLists.txt

Line 1 in 3ef35da

cmake_minimum_required(VERSION 3.25) # ubuntu 23.04 Fedora 36

[shenlebantongying]

I think searching in the build.ninja file in the build folder can confirm if /MD /MT is set.

On my machine, /MDd is automatically set.

[xiaoyifang]

Good news is that ,with the fix, the crash has gone. really mysterious.

[shenlebantongying]

The changes look fine. For merging this, I don't think we need to know the root cause, which may reveal in the future.

Old & new code should be equivalent if things work as they are documented/written anyway 😅.