Issue #276 - introduce class BSTR_Argument

src/plugin/ATL_Deprecate.cpp

 /**
  * \file ATL_Deprecate.cpp Transient functions used during the ATL removal process.
  */

[sergei, 2014/07/28 09:18:26]

This comment looks useless for me. Put our licence instead.

[Eric, 2014/07/29 16:59:27]

It's a doxygen comment.

[sergei, 2014/07/30 10:10:54]

I see that it's doxygen comment, but it's not a reason we need it.

Firstly the comments should explain non trivial things and reasons of the
application, some peculiarities of the implementation, restrictions.

Pure textural repeating of the code as well as additional lines like "/*
<empty>" makes only the noise. Arguments that it's needed for the generated
documentation don't convince, because the proper navigation in the IDE makes the
generated documentation useless, we will not use it, but will have to support
it.

[Eric, 2014/07/30 14:14:42]

I use it. Visual Studio is rather lacking in this regard.

 
 #include "ATL_Deprecate.h"
 
-std::wstring to_wstring( ATL::CString s )

[sergei, 2014/07/28 09:18:26]

1. I don't like the function without namespace. First of all there is the
function with the same name in `std` namespace, which is good and bad. Our
function should be in our namespace to avoid any ambiguity or in ATL namespace,
thus we can use ADL. What concerns `to_CString` is the same, either in our
namespace or in std.

2. pass `const ATL::CString&` and don't call `s.ReleaseBuffer();` inside this
method. To manage the lifetime of that argument is the responsibility of the
caller, any function should not know anything about the caller.

Despite this functions are intended to be temporary, anyway, all code should be
clean?

[Eric, 2014/07/29 16:59:27]

It doesn't need a namespace. The ATL::CString argument makes the call completely
unambiguous.
I've done this conversion once, I want to remind you. Since I have to do it
again now, in pieces, I'm learning from my experience. Using an explicit interim
conversion functions is better than the alternatives during the period when you
need to interconvert between types.
Have you read the documentation on CString??? If you call GetBuffer(), then you
need to call ReleaseBuffer() or risk interfering with other string operations.
It's also why you can't declare the argument 'const'. 
All code should be clean enough for its purpose. It does not need to be clean
enough for every conceivable purpose.

[sergei, 2014/07/30 10:10:54]

Firstly think. Why do we need to call non-const method to get the data?
Apparently, we don't have to do it, there is const operator which returns
wchar_t*.
I've checked the documentation,
http://msdn.microsoft.com/en-us/library/kt26tkzx.aspx
We don't change the buffer content, so we don't have to call it, but we really
don't have to GetBuffer().

I think that we need these methods.
The everything we need is just to say
std::wstring someVar = atlCString; // to convert ATL::CString to std::wstring
and
ATL::CString someVar = stdWString.c_str(); // to convert std::wstring to
ATL::CString

Such conversion methods are not good at all
- less functions => less code => code quality, readability and maintainability
is better
- to_wstring is a very generic name and it's not intended to be prefixed by any
namespace when it's used. It's should work with ADL, so the caller does not care
about the namespace in which the source is defined. Again, less code, less
dependency on the particular namespace => the everything is better.
- Even more, if we need a really good unified conversion functions, then we
should choose another approach when they accept the template argument of the
destination type, like convertTo<ATL::CString>(someVar) or
convertTo<std::wstring>(someVar). In this case they are very convenient not only
for manual using, but also for the library code like
convertTo<CurrentlyDesiredDestinationType>(someVar) where
CurrentlyDesiredDestinationType is not known when the library is being
developed. But in this particular case it's a tremendous overkill.

[Eric, 2014/07/30 14:14:42]

That's for a different class! 'CString' and 'CSimpleString' are not the same
class.
Here's the documentation for 'CString::GetBuffer'
http://msdn.microsoft.com/en-us/library/aa314880(v=vs.60).aspx

Apparently, though, CString does define 'operator LPCTSTR', which I have missed
on half a dozen passes through the CString documentation over the last several
weeks.
The whole point of a transient conversion function is to be more verbose! You
want something textually distinctive that becomes undeclared at the end of the
cycle so that you can remove the transient function completely.

Maintainability is just not an issue. The lifetime of these functions should be
at most, about a month, speedy review willing.

If you want to avoid seeing the impurity that are transient conversion
functions, please see the previous review
http://codereview.adblockplus.org/5750789393874944/ where there are no ugly,
misshapen abominations that are expedient for such intermediate code as shown
here.

+std::wstring to_wstring(const ATL::CString& s)
 {
-  std::wstring result( s.GetBuffer() );
-  s.ReleaseBuffer();
+  std::wstring result(static_cast<const wchar_t *>(s));
   return result;
 }
 
-
-ATL::CString to_CString( const std::wstring& s )

[sergei, 2014/07/28 09:18:26]

It's in every code review. Stop using spaces here and in another places.

+ATL::CString to_CString(const std::wstring& s)
 {
-  return ATL::CString( s.c_str() );
+  return ATL::CString(s.c_str());
 }