Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean: port away from QTextCodec in slob.cc and clean up iconv #1734

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

shenlebantongying
Copy link
Collaborator

@shenlebantongying shenlebantongying commented Aug 28, 2024

Another attempt to remove usage of Core5Compat.

Is this change correct?

The Qt5 version of QTextCodec::codecForName underneath uses ICU

GNU libiconv has a large table of encoding alias

https://git.savannah.gnu.org/gitweb/?p=libiconv.git;a=blob;f=lib/encodings.def;h=e5063d329886818033e6d3aa14618c75d8b07852;hb=HEAD#l441

ICU also has a big table of encoding alias

https://github.com/unicode-org/icu/blob/1414e80f6c1376afa65eb388aaaafb53169fc3dc/icu4c/source/data/mappings/convrtrs.txt#L891

Both includes names from various sources like INNA, glibc, java, windows, so I assume QTextCodec::codecForName and iconv_open are equal.

unrelated

QStringConverter since Qt6.5 also uses ICU to support equal set of encodings like old QTextCodec btw.

Maybe the correct path forward is just use ICU instead, because multiple dependencies of GD already depends on it one way or another.

@shenlebantongying
Copy link
Collaborator Author

Also, the original code appears to be mem leaking.

@shenlebantongying shenlebantongying force-pushed the clean/qtextcodec branch 3 times, most recently from c4a60a8 to d55df8e Compare August 28, 2024 10:53
src/common/iconv.hh Outdated Show resolved Hide resolved
@xiaoyifang
Copy link
Owner

USE_ICONV can be removed and default to true

src/common/iconv.hh Outdated Show resolved Hide resolved
@shenlebantongying shenlebantongying force-pushed the clean/qtextcodec branch 6 times, most recently from 9fd65f1 to 097840f Compare August 30, 2024 08:05
@shenlebantongying shenlebantongying changed the title clean: port away from QTextCodec in slob.cc clean: port away from QTextCodec in slob.cc and clean up iconv Aug 30, 2024
Copy link

sonarcloud bot commented Aug 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@xiaoyifang xiaoyifang merged commit 5549d36 into xiaoyifang:staged Aug 31, 2024
7 of 8 checks passed
@shenlebantongying shenlebantongying deleted the clean/qtextcodec branch September 4, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants