Issue 5070706781978624: Issue #276 - introduce class BSTR_Argument

Eric

Note that CComBSTR was converted to CString rather than std::wstring. This is to prevent cascading ...

July 25, 2014, 9:42 p.m. (2014-07-25 21:42:16 UTC) #1

sergei

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp File src/plugin/ATL_Deprecate.cpp (right): http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode3 src/plugin/ATL_Deprecate.cpp:3: */ This comment looks useless for me. Put our ...

July 28, 2014, 9:18 a.m. (2014-07-28 09:18:25 UTC) #2

Eric

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp File src/plugin/ATL_Deprecate.cpp (right): http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode3 src/plugin/ATL_Deprecate.cpp:3: */ On 2014/07/28 09:18:26, sergei wrote: > This comment ...

July 29, 2014, 4:59 p.m. (2014-07-29 16:59:26 UTC) #3

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/ATL_Deprecate.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:3: */
On 2014/07/28 09:18:26, sergei wrote:
> This comment looks useless for me. Put our licence instead.

It's a doxygen comment.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring( ATL::CString s )
On 2014/07/28 09:18:26, sergei wrote:
> 1. I don't like the function without namespace. [...]
> thus we can use ADL.

It doesn't need a namespace. The ATL::CString argument makes the call completely
unambiguous.

> What concerns `to_CString` is the same, either in our
> namespace or in std.

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.

> don't call `s.ReleaseBuffer()

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'. 

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

All code should be clean enough for its purpose. It does not need to be clean
enough for every conceivable purpose.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:12: BSTR_Argument::BSTR_Argument( const std::wstring& s
)
On 2014/07/28 09:18:26, sergei wrote:
> I recommend to nullify bstr member here.

Redundant. Every execution path already assigns it.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:22: bstr = nullptr;
On 2014/07/28 09:18:26, sergei wrote:
> The comment should be in the header near the ctr, as well as the test for this
> ctr is required.

This is an implementation detail, not some basic property of the class. It's a
cleaner way of detecting allocation errors than others.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:48: // Assign the empty string, if not already empty.
On 2014/07/28 09:18:26, sergei wrote:
> It's not only semantically unexpected

Directly assigning to internal members of smart pointer classes is what's
semantically unexpected by C++. This is a hack that Microsoft propagated, and at
this point there's little to do but cope with it.

Treating & as assignment-by-zero in preparation for another assignment is
perfectly fine. And it will be plenty expected in this code base, since we've
got BSTR, VARIANT, and COM pointers that all need this treatement.

> but also looks unsafe.

"Looks"? It maintains the life cycle invariants of the underlying value. How is
that unsafe?

> First of all put the assert here, it should not happen.

This is recommendation to duplicate a defect in Microsoft implementations of
these kinds of classes. In particular, CComPtr uses an assertion that compiles
to nothing-at-all in Release configuration, meaning that the implementation is
defective.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.h (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.h:62: BSTR_Argument( const std::wstring& s );
On 2014/07/28 09:18:26, sergei wrote:
> explicit

Why? There's no need for it. There's no context where there will ever be any
ambiguity. This class will never be an argument to anything.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.h:75: operator BSTR() const
On 2014/07/28 09:18:26, sergei wrote:
> operator const BSTR() const

There's no such thing as a 'const BSTR'. Tried it before; it causes all sorts of
havoc with respect to the rest of the Microsoft declarations.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/PluginClass.cpp:737: if
(SUCCEEDED(pBrowser->get_LocationURL(&result)) && !result.operator
std::wstring().empty())
On 2014/07/28 09:18:26, sergei wrote:
> Add and call the method `Length` instead of such conversion.

Absolutely not. BSTR_Argument is a conversion class. I'm adamantly opposed to
going the Microsoft way with this and giving the class semantic properties. That
way lies simply reimplementing CComBSTR and dealing with two string classes.

Part of the ATL removal process is creating wrapper objects for COM objects
(here IBrowser2) that isolate type conversions. There will be an encapsulating
method "LocationURL" that returns std::wstring. The check with empty() will be
in the final version of the code.

Consult the original version of the ATL removal to see what the code looks like
with these wrapper functions in place.
http://codereview.adblockplus.org/5750789393874944/

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:28:
CFilterElementHideAttrSelector::CFilterElementHideAttrSelector() :
m_type(TYPE_NONE), m_pos(POS_NONE), m_attr(L"")
On 2014/07/28 09:18:26, sergei wrote:
> CString should have a default constructor, just remove `, m_attr(L"")`

Done.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:527: // reuse of 'result' is not an error, since
address-of operator permits it
On 2014/07/28 09:18:26, sergei wrote:
> It's too specific and requires additional comment each time.

It will also disappear by itself, since these are both calls that will convert
to wrapper calls that encapsulate the type conversion. What are now two type
conversion in the same context will be in different function bodies.

sergei

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp File src/plugin/ATL_Deprecate.cpp (right): http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode3 src/plugin/ATL_Deprecate.cpp:3: */ I see that it's doxygen comment, but it's ...

July 30, 2014, 10:10 a.m. (2014-07-30 10:10:54 UTC) #4

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/ATL_Deprecate.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:3: */
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.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring( ATL::CString s )
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
> If you use the pointer returned by GetBuffer to change the string contents,
you must call ReleaseBuffer before you use any other CSimpleStringT member
methods.
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.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:12: BSTR_Argument::BSTR_Argument( const std::wstring& s
)
Remove `bstr = nullptr;` when `length == 0` and don't care whether all execution
paths assign it or not, whether something happens in the ctr or not. It works in
this constructor, but it's a dirty style.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:48: // Assign the empty string, if not already empty.
>> It's not only semantically unexpected
>Directly assigning to internal members of smart pointer classes is what's
semantically unexpected by C++. This is a hack that Microsoft propagated, and at
this point there's little to do but cope with it.

The right context is "I don't like the changing of the value when the address is
taken. It's not only semantically unexpected...". Changing the variable value
when the address is taken is unexpected.

> Treating & as assignment-by-zero in preparation for another assignment is
perfectly fine. And it will be plenty expected in this code base, since we've
got BSTR, VARIANT, and COM pointers that all need this treatement.

It's not fine because when somebody sees `&someVar` it's not expected to have
any side-effect here. The code should not force the reader to check the type of
all variables and to keep in mind all peculiarities of each type.

>> First of all put the assert here, it should not happen.

>This is recommendation to duplicate a defect in Microsoft implementations of
these kinds of classes. In particular, CComPtr uses an assertion that compiles
to nothing-at-all in Release configuration, meaning that the implementation is
defective.

Again, the right context here is "First of all put the assert here, it should
not happen. Secondly, return nullptr or throw an exception."
In release and in debug it will behave absolutely identically, return nullptr or
throw an exception. It's not necessary to have 'assert' which triggers the
debugger and rely on it, it can raise some even in another system component, it
can write the log message or it can be disabled. Anyway with the default assert
implementation it's impossible to correctly write tests.
I would like to return nullptr, because it can be difficult to deal with
exceptions. All functions firstly should check the incoming arguments, if the
pointer is nullptr which should not be a nullptr then the function returns
invalid argument error which is absolutely good because the caller always should
check the return value and there is a plenty of reasons why any function can
return an error.

So it's not a recommendation to duplicate a defect in MS imlp, it's an
improvement proposal without introducing any side effect.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.h (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.h:62: BSTR_Argument( const std::wstring& s );
> This class will never be an argument to anything.
No assumptions.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.h:75: operator BSTR() const
I've tried it also and it works.
struct X {
  operator const BSTR() const;
};

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:527: // reuse of 'result' is not an error, since
address-of operator permits it
Sorry, not clear.

Eric

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp File src/plugin/ATL_Deprecate.cpp (right): http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/plugin/ATL_Deprecate.cpp#newcode3 src/plugin/ATL_Deprecate.cpp:3: */ On 2014/07/30 10:10:54, sergei wrote: > Arguments that ...

July 30, 2014, 2:14 p.m. (2014-07-30 14:14:42 UTC) #5

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/ATL_Deprecate.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:3: */
On 2014/07/30 10:10:54, sergei wrote:
> 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.

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

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/ATL_Deprecate.cpp:7: std::wstring to_wstring( ATL::CString s )
On 2014/07/30 10:10:54, sergei wrote:
> I've checked the documentation,
> http://msdn.microsoft.com/en-us/library/kt26tkzx.aspx

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.

> Such conversion methods are not good at all
> - less functions => less code => code quality, readability and maintainability
> is better

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.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:12: BSTR_Argument::BSTR_Argument( const std::wstring& s
)
On 2014/07/30 10:10:54, sergei wrote:
> it's a dirty style.

If you initialize something in the constructor initialization, that tends to say
that that's what its value is. This constructor does not initialize 'bstr' that
way, and to my eye it's more confusing to have a redundant initializer that is
usually not the initial value.

If you want a perfectly clean version, it will be more verbose, because it
entails using a constructor-helper function that returns the initial value of
'bstr'. But that's just extra textual overhead.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.cpp:48: // Assign the empty string, if not already empty.
In response to this comment, I have improved the documentation in the header
file. It was unclear what the contract was.

I should point out, because it's not obvious, that the usage pattern of this
class and the other interface classes like it are as follows.
{
  interface_class instance;
  API_call( parameters, &instance );
  std::type result_value = instance;
}
Their lifetimes are short, and they'll always be used in proximity to an API
call. The implementation of 'operator&' just needs a behavior for uses outside
this pattern that's better than what Microsoft does with 'CComPtr' etc.

On 2014/07/30 10:10:54, sergei wrote:
> The right context is "I don't like the changing of the value when the address
is
> taken. It's not only semantically unexpected...". Changing the variable value 
> when the address is taken is unexpected.

Not for these classes, because we're changing the expectation.

The way the Microsoft idiom works, 'operator&' is how you perform assignment.
It's a low-quality kind of assignment, but that's out of my hands. As soon as
you view it as part of the assignment idiom, it's very clear that the code needs
to have something in common with 'operator=', which means that it releases its
internal resources in preparation for a new value.

> It's not fine because when somebody sees `&someVar` it's not expected to have 
> any side-effect here.

Expectations are not part of the language. They are part of how the language is
used.

The only place in the code this class will ever be declared, though, is
immediately before Microsoft API functions. There won't be any other place to
use the code, since by _de facto_ policy, we won't be using BSTR anywhere in the
code but as interface objects with API.

> In release and in debug it will behave absolutely identically, return nullptr
or
> throw an exception.

Returning a null value makes for a bad interface. If you want something that's
contrary to expectations, having the address-of operator applied to a valid
object return an invalid pointer is surely in that category. Furthermore, it
puts a burden on the caller to check for null and it's just asking for null
pointer exceptions that arise from skipping that check. 

Throwing an exception would be better, but it just puts a different kind of
burden on the caller. It's also not one the code is in a position to tolerate
well.

Neither of these is necessary, though. The interface is that 'operator&' never
throws, always returns a valid pointer, and behaves slightly unusually if you
need to reuse an instance (which is rare to begin with).

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/COM_Value.h (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/COM_Value.h:75: operator BSTR() const
On 2014/07/30 10:10:54, sergei wrote:
> I've tried it also and it works.
> struct X {
>   operator const BSTR() const;
> };

I did not say you couldn't declare it. What I said is that if you do declare it
wreaks havoc when you start using it.

I also said that I have been down that road. If you don't trust my experience,
perhaps you might develop some of your own. I assure you it will be nothing but
frustration.

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
File src/plugin/PluginFilter.cpp (right):

http://codereview.adblockplus.org/5070706781978624/diff/5629499534213120/src/...
src/plugin/PluginFilter.cpp:527: // reuse of 'result' is not an error, since
address-of operator permits it
On 2014/07/30 10:10:54, sergei wrote:
> Sorry, not clear.

What is your question?

Eric

Aug. 4, 2014, 2:50 p.m. (2014-08-04 14:50:29 UTC) #6

Closing this issue. Split up. See issue description for superceding reviews.

Issue 5070706781978624: Issue #276 - introduce class BSTR_Argument (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : small correction over patch set 2 #

Patch Set 4 : clarified documentation for operator&, updated to_wstring(CString) #

Messages