1768758 - nsTString::get() is awkward on Windows

Status: RESOLVED FIXED

(Core :: String, enhancement)

Description

[Ray Kraesig [:rkraesig]]

On Windows, char16_t and wchar_t are representationally-compatible but formally-distinct types. This means that %ls would work, and historically has worked, to print or log the result of nsString::get() on Windows; but with the enabling of -Wformat on Windows (bug 1766561), that now generates an error.

This error has been papered over in a number of places with static_cast<const wchar_t*>, but general adoption of such a pattern invites the risk of a primarily-Windows-side programmer doing this in cross-platform code, unaware of the danger.

Ideally there would be some platform-specific way to get an LPCWSTR without an explicit cast at the callsite. (Or, if there already is one that I'm just missing, it should probably be documented in the XPCOM string guide.)

Comment 1

[Ray Kraesig [:rkraesig]]

The changes desired here are relatively straightforward, so this is probably a good-first-bug. Interested contributors are encouraged to start by reading "How To Contribute Code To Firefox".

Assigning myself as mentor, since I originally filed the bug. (:nika, if you'd prefer that the mentor also be a valid reviewer for this module, feel free to replace me.)

Comment 2

[Weilon[:whale]]

Hey there. I'd like to take a stab at this if nobody else is working on it. If I understand correctly, the get() function is defined in xpcom/string/nsTString.h:

  MOZ_NO_DANGLING_ON_TEMPORARIES typename raw_type<T, int>::type get() const {
    return this->mData;
  }

What I need help understanding is how exactly nsTString is being used. Originally I was thinking that we could add a preprocessor directive to differentiate between Windows and other OS's (i.e. #ifdef _WIN32), and cast accordingly.

But it looks like the get function is returning raw data. Am I correct in saying that users of nsTString would nee to cast the returned data as wchar_t or char16_t?

As a side note, I'm a bit green when it comes to C++, so apologies if this is a simple question.

Thanks!

Comment 3

[Ray Kraesig [:rkraesig]]

(In reply to Weilon[:whale] from comment #2)

What I need help understanding is how exactly nsTString is being used.

nsTString is like C++'s own std::basic_string: it is the basic type which provides functionality for both wide strings (nsString) and narrow strings (nsCString), and also for a number of other string types. (See the guide here — although note that its claim of "completely separate base types" for wide and narrow strings is only true as far as the inheritance and downcasting hierarchy is concerned, as they are both instances of nsTString (et al.) with different type arguments.)

 

Originally I was thinking that we could add a preprocessor directive to differentiate between Windows and other OS's (i.e. #ifdef _WIN32), and cast accordingly.

This is probably not a viable approach. All of our cross-platform code uses char16_t for wide strings, so this would almost certainly cause breakage everywhere that get() is used in cross-platform code.

Also, IIRC, on Windows typename raw_type<T, int>::type actually resolves to the char16ptr_t type defined in mfbt/Char16.h; callers can already use .get() with both Windows LPCWSTR-expectant APIs and cross-platform char16_t*-expectant APIs. The problem is that this doesn't play well with C-style variadic functions... which is what most of our logging functions are.

The approach I would suggest is simply adding a Windows-only getW() member function to nsTString which a) unconditionally returns wchar_t* and b) uses SFINAE shenanigans in order to not exist on non-wide-character instantiations of nsTString.

(The customary preprocessor directive for the Firefox codebase would be #ifdef XP_WIN, incidentally.)

Comment 4

[Weilon[:whale]]

Attachment: Bug 1768758 - Add Windows-specific function getW() for getting wchar_t

Comment 5

[Weilon[:whale]]

(In reply to Ray Kraesig [:rkraesig] from comment #3)

Thanks for the advice. I've submitted a patch for review. I'm not sure who I should tag as a reviewer, so could you do that for me?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/579a277b2eae
Add Windows-specific function getW() for getting wchar_t r=xpcom-reviewers,win-reviewers,gstoll,nika

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/579a277b2eae

Comment 8

[Weilon[:whale]]

Thanks for accepting and merging the patch!

Comment 9

[Ray Kraesig [:rkraesig]]

No, thank you for writing and submitting it!