Skip to content
Snippets Groups Projects

qt: do not assume temporary file name can be encoded with Latin1

Open Fatih Uzunoğlu requested to merge fuzun/vlc:qt/rvaluetopointer into master
2 unresolved threads

Request review @chub.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Pierre Lamot
  • Pierre Lamot changed milestone to %4.0

    changed milestone to %4.0

  • Fatih Uzunoğlu added 2 commits

    added 2 commits

    • 9a1aface - qt: do not convert rvalue into pointer in ModelRecoveryAgent
    • 8a9c01de - qt: define QT_NO_CAST_FROM_BYTEARRAY

    Compare with previous version

  • Fatih Uzunoğlu added 2 commits

    added 2 commits

    • 219a390a - qt: do not convert rvalue into pointer in ModelRecoveryAgent
    • 29f99af7 - qt: define QT_NO_CAST_FROM_BYTEARRAY

    Compare with previous version

  • Fatih Uzunoğlu approved this merge request

    approved this merge request

  • Prince Gupta
    • Author Reporter

      @chub Did you want to use QString::toLocal8Bit() (I now remembered the existence of this), and QByteArray::constData(), and keep using rename() and remove() instead of std::filesystem?

    • Author Reporter

      On Windows, I'm not sure if rename() functions properly with the used code page if the used code page is UTF-8. QString::toLocal8Bit() is also noted to not have a defined behavior if the string can not be represented by local 8-bit encoding.

      As far as I know, the temporary file path may be Unicode, even though the local code page is not Unicode. In that case, using QString::toLocal8Bit() would result in undefined behavior.

      Even if the temporary file path is Unicode and the local code page is UTF-8, I'm not sure if MSVCRT supports UTF-8 with char*.

      For Win32 API [1], it seems that functions accepting char* (instead of wchar*) can support UTF-8:

      Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.

      [1] https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page#-a-vs--w-apis

    • maybe use vlc_rename/vlc_unlink they expect utf8 string and perform the conversion if needed.

    • from what I'm understanding std::filesystem::path is supposed to do the conversion, though (former version of) gcc seems to be bugged, I reproduce this crash on my machine:

      https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95048

    • Author Reporter

      If we have to choose between converting to UTF-8 or UTF-16, UTF-16 makes more sense because QString is UTF-16. That's why I used QString::toStdU16String() and let the path get constructed from std::u16string.

      from what I'm understanding std::filesystem::path is supposed to do the conversion

      We use std::u16string, can you try constructing from that instead of wide character literal?

    • Please register or sign in to reply
  • Fatih Uzunoğlu added 61 commits

    added 61 commits

    • 29f99af7...456d7c92 - 59 commits from branch videolan:master
    • 29d81552 - qt: do not convert rvalue into pointer in ModelRecoveryAgent
    • d24d282e - qt: define QT_NO_CAST_FROM_BYTEARRAY

    Compare with previous version

  • Jean-Baptiste Kempf requested review from @chub

    requested review from @chub

  • Fatih Uzunoğlu added 241 commits

    added 241 commits

    • d24d282e...3416bca8 - 239 commits from branch videolan:master
    • f5fd5f01 - qt: do not convert rvalue into pointer in ModelRecoveryAgent
    • 3b31423d - qt: define QT_NO_CAST_FROM_BYTEARRAY

    Compare with previous version

  • Fatih Uzunoğlu mentioned in merge request !6335 (merged)

    mentioned in merge request !6335 (merged)

  • Fatih Uzunoğlu changed title from qt: do not convert rvalue into pointer and disable implicit conversion to qt: do not assume temporary file name can be encoded with Latin1

    changed title from qt: do not convert rvalue into pointer and disable implicit conversion to qt: do not assume temporary file name can be encoded with Latin1

    • Author Reporter

      @robUx4 I will wait until minimum GNU/LLVM is bumped to 9.1/9, so that I can use std::filesystem without directing the linker to link filesystem explicitly.

    • I don't think we will ever use std::filesystem we have filesystem API's that abstract the system. We should use that instead. If they are wrong, we need to fix them, not keep broken API's and use something else in C++.

    • Author Reporter

      I don't get why we can not use a standard C++ feature just to rely on in-house abstraction. The incentive to fix a broken part should not arise from realizing something else is doing it properly. If some part is broken, it needs to be fixed regardless.

      I'm not even sure how to handle the encoding properly with POSIX. libiconv does not seem to be mandatory for VLC.

    • We don't really have any reason not to use std::filesystem (specifically since this is standard) as long as it works everywhere (compat or system impl), just like we didn't have to keep our bit manipulation code as soon as C23 exposed a dedicated standard API for that.

      The main problem is that writing a polyfill for C++ template code that is multiplatform is much much more demanding than what we see in C.

    • Please register or sign in to reply
  • Please register or sign in to reply
    Loading