Issue #276 - introduce classes BSTR_ParamArgument and BSTR_ParamResult

Issue #276 - introduce classes BSTR_ParamArgument and BSTR_ParamResult.

New classes BSTR_ParamArgument and BSTR_ParamResult. Eliminate all uses of
CComBSTR when it appears as an argument to a COM function.

Depends upon http://codereview.adblockplus.org/6224768520945664/

Supersedes in part http://codereview.adblockplus.org/5070706781978624/

[Oleksandr, 2014-08-05 13:12:39]

I'm having doubts if we should still push the CComBSTR replacement. It seems
like it's quite an extensive change with very little win. Would you agree?

[sergei, 2014-08-05 13:35:07]

I agree. For me it requires even more efforts to support it than it benefits.

[Eric, 2014-08-05 14:19:48]

On 2014/08/05 13:12:39, Oleksandr wrote:
> [...] quite an extensive change with very little win. Would you agree?

No, but that's hardly surprising. I've worked with these interface classes for
several weeks now, and there are good reasons to have them.

The most fundamental reason is that wherever BSTR occurs, there are two things
you have to do with it: manage its life cycle and interconvert with
std::wstring. Putting these two responsibilities into a single class makes
sense. If CComBSTR did interconversion with std::wstring, there would be far
less reason to write our own. CComBSTR does interconvert with CString reasonably
well, because Microsoft wrote them to behave so.

One good reason is that there is already an existing BSTR interface class in the
code base. It's "BString" in PluginUtil.h. All it does is a bit of life cycle
management. The present classes do that plus type conversion.

Another of the good reasons is that dealing with null-or-empty BSTR. Both
represent the empty string; there's no such thing as "no such string" with BSTR.
There are one or two of minor defects in the current code where they aren't
treated identically and the consequence matters. I don't recall where they are,
since using a proper interface class just fixes them. I don't even recall
whether they're even in the present change set, since not all BSTR occurrences
are using CComBSTR. So since our goal is to move the code base to use standard
types, you're going to need at least some common conversion functions.

The place where these classes really shine, though, is when you isolate the API
calls they're using it to wrapper classes on the COM objects. The methods of
such classes provide interfaces using standard types (primarily strings) to the
API calls. After that set of changes, these classes only appear in the wrapper
implementations; they don't appear in the main body of code. It greatly
simplifies the main code; I've seen it, and it's far simpler and easier to
follow, because there's no longer any intermixing of type conversion with the
algorithm. It also simplifies the wrapper implementations, since they're totally
idiomatic with the right interface classes.

[Oleksandr, 2014-08-05 16:23:52]

I would agree with this if we only had one class handling BSTRs. Original
discussion on this can be found here:
http://codereview.adblockplus.org/5750789393874944/diff/5629499534213120/src/...

I think this multiple classes approach is a premature optimization. We don't
really have much of a speed problems. And in cases where we do need to be faster
(DOM traverser) we should look into algorithms better in terms of Big Oh
notation, not simply decreasing a constant multiplier of a run time. 

On the flip side, if a person joins the project in future it will be much easier
for him/her to understand the code with CComBSTR (a usual way of dealing with
BSTRs) or with our direct CComBSTR replacement vs the code with multiple
non-standard BSTR classes.

My comments below.


> The most fundamental reason is that wherever BSTR occurs, there are two things
> you have to do with it: manage its life cycle and interconvert with
> std::wstring. 

CComBSTR does life cycle management as well and I don't see any problems at all
in something like:

CComBSTR foo = ...;
std::wstring bar = std::wstring(foo, foo.Length());

Not too different from the case with CString, IMHO.

> It's "BString" in PluginUtil.h. 

We should actually remove that class and change it to CComBSTR.

> Another of the good reasons is that dealing with null-or-empty BSTR. 

Sounds to me as if fixing the places where we deal with such strings incorrectly
is still a better solution.

[Eric, 2014-08-05 17:30:23]

On 2014/08/05 16:23:52, Oleksandr wrote:
> I would agree with this if we only had one class handling BSTRs.
[...]
> I think this multiple classes approach is a premature optimization.

Here's the history:
- First version in the big, first ATL review:
http://codereview.adblockplus.org/5750789393874944/. This one used two classes.
- Second version in the review this one superseded:
http://codereview.adblockplus.org/5750789393874944
- The one here is the the third version.

In the first version, I separated them out based on (essentially) a hunch that
it would work better that way. The second version, I took the advice to combine
them and it worked initially. However, it stopped working well as I continued to
convert over API calls. I started having problems with implicit conversions to
wchar_t *. I didn't post that code for review because it's not good code, and
it's not good code because having both conversion functions in this class is not
a good design. As a result, I went back to using separate classes for
...Argument and ...Result because the single class design does not actually work
in practice, however desirable it might be in theory.

So it's absolutely not a premature optimization. It's a design changed as a
result of experience.

I did document this design choice in the current code. See line 45 in this file:
http://codereview.adblockplus.org/5140323101573120/diff/5629499534213120/src/...
I did update the class documentation for this version, although I didn't need to
alter all of the text.

> We don't really have much of a speed problems.

I don't think I've ever made an argument for adoption on the basis that these
classes are faster. If you read something in the documentation as implying that,
tell me and I'll reword it to make it clearer.

I doubt they are faster, in fact, since we have to make a BSTR copy for every
argument. There are some savings elsewhere from avoiding extraneous copies from
tossing everything into CComBSTR by policy. The net change is unlikely to be
much.

> On the flip side, if a person joins the project in future it will be much
easier
> for him/her to understand the code with CComBSTR (a usual way of dealing with
> BSTRs) or with our direct CComBSTR replacement vs the code with multiple
> non-standard BSTR classes.

Frankly, I don't really believe this. These classes should only appear around
API class, and in a very idiomatic way. When the API calls themselves are
isolated into wrapper methods, the code gets even cleaner. Take a peek at the
wrapper classes in the original code review. See, for example, line 180 for the
wrapper around 'get_className'
http://codereview.adblockplus.org/5750789393874944/diff/5629499534213120/src/...
(Incidentally, all the ternary operators are an artifact of the old version; all
you need is simple assignment in the present version.)

And if you'd like to see why wrapped API calls are desirable, see the following
example, as well as the code in its vicinity. It's both shorter and easier to
read. 
http://codereview.adblockplus.org/5750789393874944/diff/5629499534213120/src/...

> I don't see any problems at all in something like:
>
> CComBSTR foo = ...;
> std::wstring bar = std::wstring(foo, foo.Length());

That code is defective. The implicit conversion of 'foo' to wchar_t * for
argument 1 of the std::wstring constructor can return a null pointer. A null
pointer argument here is specified as having undocumented behavior.

The upshot is that either you have to explicitly check for null everywhere
(repeated code and bad for maintenance) or you need a conversion function that
performs the check correctly and concisely.

> > Another of the good reasons is that dealing with null-or-empty BSTR. 
> 
> Sounds to me as if fixing the places where we deal with such strings
incorrectly
> is still a better solution.

One issue is incorrect use. There are, also, however a number of uses dealing
with null-or-empty that look like they might be incorrect, but aren't. The code
is easier to read when the wrapper returns std::wstring and you invoke empty()
on it. It reads better, it will never have conversion defects, and it's easier
to audit.

[sergei, 2014-08-06 12:54:21]

I also don't find multiple classes approach here to be useful.

Mainly, because it's based on assumptions how they are used. The another
important point is it will be confusing for another new people. They may or may
not be acquainted with ATL, but there are a lot of resources regarding ATL and
about its dangerous places, on another hand it's the fact that nobody else knows
about these our classes and how they should be used.

So, in my opinion, the best way is to use ATL::CComBSTR and have a conversion
function, or if we decide to get rid of ATL, replace it by one similar class
which prevents the wrong usage like in the example above (example of a potential
mistake), thus there should not be `operator wchar_t*()` (or `operator BSTR()`).

JIC, designing that class we should provide only used methods (no dead code)
which are safer by API than methods in the ATL class. And we should not
introduce any side effects.

What concerns the conversion to std::wstring. Could you please point to the
official source that "BSTR makes no distinction between null pointers and
non-null pointers to the empty string". There is an obvious distinction for C++
and C# clients, null means the absence of string, zero length string means an
empty string. I've also tested for JScript. It's not allowed to pass null to the
method which expects BSTR and the value of BSTR for the empty string argument ""
is the empty BSTR string, not nullptr. For the return value, when it is set to
nullptr, the value in JScript is the empty string "". There might be the
influence of implementation details of conversion from JScript object to VARIANT
and vice versa for BSTR and BSTR* argument types, and the influence of the
ECMAScript standard which does not allow to have a string type variable with the
null value, the type of null value is Null type.
Even more, SysAllocString
http://msdn.microsoft.com/en-us/library/windows/desktop/ms221458(v=vs.85).aspx
which is used to allocate BSTR string does make the distinction, "If psz is a
zero-length string, returns a zero-length BSTR. If psz is NULL or insufficient
memory exists, returns NULL."
So, I would say it depends on the context of the domain, to have the ability to
distinguish between null string and the empty string looks good to me.

Anyway, if we stick to the rule, that if the input is an empty string, then the
value of BSTR string is null pointer, so I think it would be expected to have
the empty std::wstring if BSTR is nullptr. It can be implemented as the
`operator std::wstring() const`, like it is in the current code review, or it
can be a function `std::wstring toWString(const BSTR value)`.
Cast operators in C++ are very useful, but at the same time they require a lot
of attention. I find to have explicit methods which provide the access to the
internals of the class to be much safer, despite the compiler will not warn
about wrong usage. The safeness comes from the code review. If there is
something like `someVar.someHandle()` it's natural to expect the return value to
be an invalid value and it should be checked before using. What concerns the
requirement to check for the null everywhere, it's beyond this issue, all
functions should check the validity of all incoming arguments before using
without any assumptions about the caller, there is no other way.

tl;dr
One class is better. If we use ATL::CComBSTR then we need a conversion functions
with the comment regarding handling null case. If it is our own class, then
instead of `operator BSTR()` provide `operator std::wstring() const` with the
comments about null case near the operator and near the constructor. Don't make
a side effect with `operator&`, it's a premature optimization.

[Eric, 2014-08-06 14:22:20]

On 2014/08/06 12:54:21, sergei wrote:
> Mainly, because it's based on assumptions how they are used.

As I've seen before, this is a personal zealotry of yours rather than any
well-agreed principle of programming. Sometimes you want general-purpose
classes. Sometimes you want special-purpose classes. Insisting that everything
be exactly one way is just foolishness.

> The another
> important point is it will be confusing for another new people.

Confusing? I just don't get this.
- If you pass BSTR as a parameter to a COM call, use one of the BSTR_Param*
classes.
- If that BSTR parameter acts as an argument to the call, use
BSTR_ParamArgument.
- If that BSTR parameter acts as a result (a return value) from the call, use
BSTR_ParamResult.

The interfaces of these classes reinforce the usage pattern. You can't
initialize a result type with anything but an empty value, nor can you convert
it to a BSTR, so it won't even compile as an argument. Vice-versa, you can't
take the address of an argument type, so it won't compile as a result.

And on top of that, the plan has been (since the first time I did this) to
create a single module that's the only place these would be used (wrappers for
DOM element types), so there are plenty of examples to imitate.

In short, I reject this "confusing" label.

> replace it by one similar class
> which prevents the wrong usage like in the example above

This design already prevents wrong usage, as I've just argued. Have you spent
more time looking at the code that you did writing up your critique?

> JIC, designing that class we should provide only used methods (no dead code)
> which are safer by API than methods in the ATL class.

The present classes meet exactly the two criteria you've outlined here. There's
no dead code. The API is safer, as it refuses to compile if used incorrectly.

> And we should not
> introduce any side effects.

Having zero side effects would be defective behavior. If you want a class
without side-effects that is also defective, I will object. In fact, CComBSTR is
already defective in this way.

You don't seem to have read (or if you've read it, you haven't understood it) my
analysis of the problem, and why I didn't naively replicate the Microsoft
behavior. At the very least, you haven't made any argument that the analysis is
wrong, which I expect you to do if you are weighing in against it. Therefore,
let me state it again.

The issue arises when the internal BSTR argument is not-null and operator& is
called on it. In the present case, where we are calling a COM function, we have
the responsibility for releasing the BSTR. If the BSTR is not released, it
creates a system memory leak that's cured only by rebooting. (This is just
horribly bad design by Microsoft, and we have to live with it.) So in all cases
we have to release the BSTR. This is the side effect you are referring to, that
operator& has to release a non-trivial BSTR if it has one. It then has to
replace that value (so it's not released again), so it replaces it with nullptr,
the closest thing BSTR has to no-such-value. If you don't release the BSTR in
that case, you have introduced a system memory leak and your code is defective.

The alternatives are each defective. We've already discussed two: using the
moral equivalent of ATLASSERT (what's in the Microsoft code) and throwing an
exception. In both cases, you still haven't released the BSTR. Even calling
std::terminate() is defective, which is kind of astonishing; it doesn't release
the BSTR either, and the memory leak happens in the OS, not in the application
code. As a reminder, this memory leak is a system resource leak that's only
cured by rebooting the machine.

The only correct behavior is to call SysFreeString() on a non-empty BSTR during
operator&, and that's a side effect. Your criterion of "no side effect" is
impossible without making defective code.

> What concerns the conversion to std::wstring. Could you please point to the
> official source that "BSTR makes no distinction between null pointers and
> non-null pointers to the empty string".

The official sources are horribly convoluted and confusing, as per much
Microsoft documentation. Mostly it doesn't even address the issue (typical). See
the following for useful information that's gleaned from long experience with
these.
stackoverflow : Should there be a difference between an empty BSTR and a NULL
BSTR?
http://stackoverflow.com/questions/171641/should-there-be-a-difference-betwee...
Eric Lippert's Blog : Eric's Complete Guide To BSTR Semantics
http://blogs.msdn.com/b/ericlippert/archive/2003/09/12/52976.aspx

> Even more, SysAllocString
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms221458%28v=vs.85%29...
> which is used to allocate BSTR string does make the distinction, "If psz is a
> zero-length string, returns a zero-length BSTR. If psz is NULL or insufficient
> memory exists, returns NULL."

This is the behavior as it affects representation. This behavior does not bear
on the semantics of BSTR, that is, what the value means, or more to the point,
how to write a correct conversion to std::wstring. I do trust you know the
distinction between representation and semantics, but perhaps not. (Given an
interpretation map, the representation is its domain and the semantic meaning of
the representation is its range.)

[Oleksandr, 2014-10-07 22:01:41]

Bump. Shall we close this?
On 2014/08/06 14:22:20, Eric wrote:
> On 2014/08/06 12:54:21, sergei wrote:
> > Mainly, because it's based on assumptions how they are used.
> 
> As I've seen before, this is a personal zealotry of yours rather than any
> well-agreed principle of programming. Sometimes you want general-purpose
> classes. Sometimes you want special-purpose classes. Insisting that everything
> be exactly one way is just foolishness.
> 
> > The another
> > important point is it will be confusing for another new people.
> 
> Confusing? I just don't get this.
> - If you pass BSTR as a parameter to a COM call, use one of the BSTR_Param*
> classes.
> - If that BSTR parameter acts as an argument to the call, use
> BSTR_ParamArgument.
> - If that BSTR parameter acts as a result (a return value) from the call, use
> BSTR_ParamResult.
> 
> The interfaces of these classes reinforce the usage pattern. You can't
> initialize a result type with anything but an empty value, nor can you convert
> it to a BSTR, so it won't even compile as an argument. Vice-versa, you can't
> take the address of an argument type, so it won't compile as a result.
> 
> And on top of that, the plan has been (since the first time I did this) to
> create a single module that's the only place these would be used (wrappers for
> DOM element types), so there are plenty of examples to imitate.
> 
> In short, I reject this "confusing" label.
> 
> > replace it by one similar class
> > which prevents the wrong usage like in the example above
> 
> This design already prevents wrong usage, as I've just argued. Have you spent
> more time looking at the code that you did writing up your critique?
> 
> > JIC, designing that class we should provide only used methods (no dead code)
> > which are safer by API than methods in the ATL class.
> 
> The present classes meet exactly the two criteria you've outlined here.
There's
> no dead code. The API is safer, as it refuses to compile if used incorrectly.
> 
> > And we should not
> > introduce any side effects.
> 
> Having zero side effects would be defective behavior. If you want a class
> without side-effects that is also defective, I will object. In fact, CComBSTR
is
> already defective in this way.
> 
> You don't seem to have read (or if you've read it, you haven't understood it)
my
> analysis of the problem, and why I didn't naively replicate the Microsoft
> behavior. At the very least, you haven't made any argument that the analysis
is
> wrong, which I expect you to do if you are weighing in against it. Therefore,
> let me state it again.
> 
> The issue arises when the internal BSTR argument is not-null and operator& is
> called on it. In the present case, where we are calling a COM function, we
have
> the responsibility for releasing the BSTR. If the BSTR is not released, it
> creates a system memory leak that's cured only by rebooting. (This is just
> horribly bad design by Microsoft, and we have to live with it.) So in all
cases
> we have to release the BSTR. This is the side effect you are referring to,
that
> operator& has to release a non-trivial BSTR if it has one. It then has to
> replace that value (so it's not released again), so it replaces it with
nullptr,
> the closest thing BSTR has to no-such-value. If you don't release the BSTR in
> that case, you have introduced a system memory leak and your code is
defective.
> 
> The alternatives are each defective. We've already discussed two: using the
> moral equivalent of ATLASSERT (what's in the Microsoft code) and throwing an
> exception. In both cases, you still haven't released the BSTR. Even calling
> std::terminate() is defective, which is kind of astonishing; it doesn't
release
> the BSTR either, and the memory leak happens in the OS, not in the application
> code. As a reminder, this memory leak is a system resource leak that's only
> cured by rebooting the machine.
> 
> The only correct behavior is to call SysFreeString() on a non-empty BSTR
during
> operator&, and that's a side effect. Your criterion of "no side effect" is
> impossible without making defective code.
> 
> > What concerns the conversion to std::wstring. Could you please point to the
> > official source that "BSTR makes no distinction between null pointers and
> > non-null pointers to the empty string".
> 
> The official sources are horribly convoluted and confusing, as per much
> Microsoft documentation. Mostly it doesn't even address the issue (typical).
See
> the following for useful information that's gleaned from long experience with
> these.
> stackoverflow : Should there be a difference between an empty BSTR and a NULL
> BSTR?
>
http://stackoverflow.com/questions/171641/should-there-be-a-difference-betwee...
> Eric Lippert's Blog : Eric's Complete Guide To BSTR Semantics
> http://blogs.msdn.com/b/ericlippert/archive/2003/09/12/52976.aspx
> 
> > Even more, SysAllocString
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms221458%2528v=vs.85%...
> > which is used to allocate BSTR string does make the distinction, "If psz is
a
> > zero-length string, returns a zero-length BSTR. If psz is NULL or
insufficient
> > memory exists, returns NULL."
> 
> This is the behavior as it affects representation. This behavior does not bear
> on the semantics of BSTR, that is, what the value means, or more to the point,
> how to write a correct conversion to std::wstring. I do trust you know the
> distinction between representation and semantics, but perhaps not. (Given an
> interpretation map, the representation is its domain and the semantic meaning
of
> the representation is its range.)