#83 Simplify OWL usage of different string signatures while extending those signatures

[Joe Slater]

This is a continuing discussion started in [feature-requests:#82].

Since tstring cannot be extended with operator overloading or new constructors due to various issues (see above), we have four different signatures for MessageBox in TWindow (and TApplication):

:::C++
MessageBox(LPCTSTR text, LPCTSTR caption = 0, uint flags = MB_OK);
MessageBox(const tstring& text, const tstring& caption = tstring(), uint flags = MB_OK);
MessageBox(uint resId, LPCTSTR caption = 0, uint flags = MB_OK);
MessageBox(uint resId, const tstring& caption, uint flags = MB_OK);

And yet there are several more permutations not represented:

:::C++
MessageBox(LPCTSTR text, const tstring& caption = tstring(), uint flags = MB_OK);
MessageBox(const tstring& text, LPCTSTR caption = 0, uint flags = MB_OK);
MessageBox(uint resId, uint captionResId, uint flags = MB_OK);
MessageBox(const tstring& text, uint captionResId, uint flags = MB_OK);
MessageBox(LPCTSTR text, uint captionResId, uint flags = MB_OK);

In order to prevent such convolution and allowing such convenience to OWL implementations, I propose the following…

Create a new (wrapper) class providing implicit conversions to a unified string type. Consider the following, shown without PRECONDITION/CHECK/WARN/etc. for brevity and clarity and by no means to have a complete exhaustive list of conversions:

:::C++
class TStringC {

  public:
    TStringC () : s() {}
    TStringC (TStringC& tString) : s(tString.s) {}
    TStringC (tstring& tString) : s(tString) {}
    TStringC (tstringstream& tStream) : s(tStream.str()) {}
    TStringC (tistringstream& tStream) : s(tStream.str()) {}
    TStringC (tostringstream& tStream) : s(tStream.str()) {}
    TStringC (LPCTSTR cString) : s(cString) {}
    TStringC (uint resId, TModule& module = *Module) : s(module.LoadString(resId)) {}
    TStringC (TStatic& ctlString) : s(ctlString.GetText()) {}
    TStringC (TEdit& ctlString) : s(ctlString.GetText()) {}
    TStringC (TComboBox& ctlString) : s(ctlString.GetText()) {}
    TStringC (TWindow& w) : s(w.GetCaption()) {}

    virtual ~TStringC () {}

    LPCTSTR GetC (const bool useNull = false) const { return ((useNull && s.empty()) ? Null : s.c_str()); }

    operator LPCTSTR() const { return (GetC()); }

    owl::tstring s;

    static LPCTSTR Null; // LPCTSTR TStringC::Null = static_cast<LPCTSTR>(0);

};

None of the constructors are declared explicit, and they have only one required argument thus providing implicit conversions.

This allows MessageBox in TWindow (and likewise TApplication) to have a single definition:

:::C++
int MessageBox (TStringC text, TStringC caption = TStringC(TStringC::Null), UINT flags = MB_OK);

Due to the operator overload also provided above, the ultimate Windows API call (from TApplication) could also simply be:

:::C++
::MessageBox(hWnd, text, caption.GetC(true), flags);

In short, anywhere an LPCTSTR is needed (for Windows API) but other string types would like to be provided, could be simplified with the use of TStringC.

For example, the above allows me to do the following:

:::C++
TListBox* myLB = new TListBox(this, IDC_LIST);
// …
MessageBox(*myLB, _T("Invalid Selection"));

And it is quite often messages have to be formatted, and using string streams is a common method of doing that:

:::C++
tostringstream sMsg;
// …
sMsg << _T("WideCharToMultiByte conversion failed! Return Value: ") << ::GetLastError();
MessageBox(sMsg, _T("Multi-Byte Conversion Error"));

While the creation of TStringC is mainly due for the convenience of implicit conversions so that multiple signatures can be eliminated while extended its capability of receiving different string types, I have provided it such that it can also be explicitly instantiated and manipulated (the tstring s is public), as well as used as a base class so that user-defined implementations could extend its capabilities for its own use:

:::C++
TEdit* myEdit = new TEdit(this, IDC_EDITSOMETHING);
// …
MessageBox(TStringC(*myEdit).s.append(_T(" is not a valid value.")), _T("You Have an Error!"));

And:

:::C++
class TMyAboutString : public TStringC {
  public:
    TMyAboutString (bool extendedInfo = true) : TStringC() {

      TModuleVersionInfo verInfo(*Module);

      s.append(verInfo.GetProductName())
       .append(_T("\nVersion "))
       .append(verInfo.GetProductVersion());

      if (extendedInfo) {
        s.append(_T("\nCopyright © "))
         .append(verInfo.GetLegalCopyright())
         .append(_T("\n\n"))
         .append(verInfo.GetFileDescription());
      }
    }
};
// …
void TMyApp::ShowAbout(bool extendedInfo = true) {
  MessageBox(GetMainWindow(), TMyAboutString(extendedInfo), _T("About"));
}

The only real caveat is when MessageBox is called, specified with caption set to 0 (so that explicit flags can be set):

:::C++
MessageBox(_T("Hello World"), 0, MB_OK | MB_ICONINFORMATION);

This creates ambiguity as the compiler doesn't know whether 0 represents LPCTSTR text or uint resId. This can be handled by any of the following:

:::C++
MessageBox(_T("Hello World"), (LPCTSTR) 0, MB_OK | MB_ICONINFORMATION);
MessageBox(_T("Hello World"), static_cast<LPCTSTR>(0), MB_OK | MB_ICONINFORMATION);
MessageBox(_T("Hello World"), TStringC::Null, MB_OK | MB_ICONINFORMATION);

The latter would be preferred, and I believe current such usage is minimal.

Note this same caveat exists with the present design (such usage is valid with respect to Windows API, but with OWL would generate a compile error):

:::C++
MessageBox(0);

Such usage seems highly unlikely.

I have tested within my own implementation all of the above, and it seemingly works flawlessly.

[Joe Slater]

An addendum…

With C++11, TStringC::Null isn't necessary and could be removed. All of the places that mention it above would be replaced with nullptr.

[Vidar Hasfjord]

This would be such a momentous change, I think I will have to put its consideration on the stack for now.

As first step, I would eliminate the uint overloads for resources, forcing LoadString at the call site. And, as I mentioned elsewhere, I would remove the unsafe FormatMessageBox functions altogether, forcing the formatting to be done at the call site.

That would leave overloads for tstring and LPCTSTR. Ideally, I want to remove the LPCTSTR overloads and rely on implicit tstring conversions, but unfortunately passing 0 for the caption parameter is allowed and common. That leaves:

:::C++
auto MessageBox(const tstring& msg, const tstring& caption, uint flags = MB_OK) -> int;

auto MessageBox(const tstring& msg, LPCTSTR caption = nullptr, uint flags = MB_OK) -> int;

[Vidar Hasfjord]

This creates [overload] ambiguity as the compiler doesn't know whether 0 represents LPCTSTR text or uint resId

Actually, you can disambiguate by providing an overload for int. That will provide an perfect and preferred type match for literal 0 as an argument.

But now you are in overload hell. :-)

[Joe Slater]

An addendum…

Constructors added to TStringC:

:::C++
#ifdef _UNICODE
TStringC (const char* cString, DWORD codePage = 0, DWORD conversionFlag = MB_PRECOMPOSED)
{
  auto wcSize = ::MultiByteToWideChar(
    codePage,
    conversionFlag,
    cString,
    -1,
    0,
    0
  );
  if (wcSize) {
    auto unicode = new wchar_t [wcSize];
    if (unicode) {
      auto ret = ::MultiByteToWideChar(
        codePage,
        conversionFlag | MB_ERR_INVALID_CHARS,
        cString,
        -1,
        unicode,
        wcSize
      );
      if (ret) s = unicode;
      delete [] unicode;
    }
  }
}
#else
TStringC (const wchar_t* cString, DWORD codePage = 0, DWORD conversionFlag = WC_DEFAULTCHAR)
{
  auto mbSize = ::WideCharToMultiByte(
    codePage,
    conversionFlag,
    cString,
    -1,
    0,
    0,
    0,
    0
  );
  if (mbSize) {
    auto mbString = new char [mbSize];
    if (mbString) {
      auto ret = ::WideCharToMultiByte(
        codePage,
        conversionFlag,
        cString,
        -1,
        mbString,
        mbSize,
        0,
        0
      );
      if (ret) s = mbString;
      delete [] mbString;
    }
  }
}
#endif

The above allows mixed-mode character strings in an application; tstring does not.

For example, if a Unicode application is built the following is invalid:

:::C++
tstring s = "Hello";

But the above for Unicode does allow:

:::C++
MessageBox("Hello", "Hello Message");
MessageBox(TStringC("‚±‚ñ‚É‚¿‚Í", 932), "Hello Message in Japanese");

The latter being more important, because tstring is not multi-byte-friendly; it merely treats multi-byte as single-byte.

For a non-Unicode application, multi-byte is insignificant, but it is possible to have a Unicode string (albeit unlikely) and the above would help encapsuate it.

While I have been using MessageBox as an example where different signatures exist, it is by no means the only one.

[Vidar Hasfjord]

Since we are moving in the opposite direction, i.e. we are removing implicit conversions, in line with general good coding advice, as well as having had bad experiences with bugs caused by implicit conversions, I have closed this ticket as won't-fix.

An interesting development in C++20 is that they have now implemented our idea from years ago; i.e. they now do compile-time check for null-pointer arguments to string constructors (which crashed at run-time before). This means we can soon retire more old-style function signatures with C-style string parameters.