Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5140323101573120: Issue #276 - introduce classes BSTR_ParamArgument and BSTR_ParamResult (Closed)

Created:
Aug. 4, 2014, 2:41 p.m. by Eric
Modified:
Dec. 30, 2014, 1:48 p.m.
Reviewers:
Visibility:
Public.

Description

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/

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+324 lines, -70 lines) Patch
M adblockplus.gyp View 1 chunk +2 lines, -0 lines 0 comments Download
M src/plugin/AdblockPlusDomTraverser.cpp View 3 chunks +9 lines, -10 lines 0 comments Download
A src/plugin/COM_Value.h View 1 chunk +174 lines, -0 lines 0 comments Download
A src/plugin/COM_Value.cpp View 1 chunk +70 lines, -0 lines 0 comments Download
M src/plugin/PluginClass.cpp View 5 chunks +13 lines, -12 lines 0 comments Download
M src/plugin/PluginDomTraverserBase.h View 3 chunks +7 lines, -9 lines 0 comments Download
M src/plugin/PluginFilter.h View 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/PluginFilter.cpp View 9 chunks +48 lines, -38 lines 0 comments Download

Messages

Total messages: 9
Eric
Aug. 4, 2014, 2:46 p.m. (2014-08-04 14:46:12 UTC) #1
Oleksandr
I'm having doubts if we should still push the CComBSTR replacement. It seems like it's ...
Aug. 5, 2014, 1:12 p.m. (2014-08-05 13:12:39 UTC) #2
sergei
I agree. For me it requires even more efforts to support it than it benefits.
Aug. 5, 2014, 1:35 p.m. (2014-08-05 13:35:07 UTC) #3
Eric
On 2014/08/05 13:12:39, Oleksandr wrote: > [...] quite an extensive change with very little win. ...
Aug. 5, 2014, 2:19 p.m. (2014-08-05 14:19:48 UTC) #4
Oleksandr
I would agree with this if we only had one class handling BSTRs. Original discussion ...
Aug. 5, 2014, 4:23 p.m. (2014-08-05 16:23:52 UTC) #5
Eric
On 2014/08/05 16:23:52, Oleksandr wrote: > I would agree with this if we only had ...
Aug. 5, 2014, 5:30 p.m. (2014-08-05 17:30:23 UTC) #6
sergei
I also don't find multiple classes approach here to be useful. Mainly, because it's based ...
Aug. 6, 2014, 12:54 p.m. (2014-08-06 12:54:21 UTC) #7
Eric
On 2014/08/06 12:54:21, sergei wrote: > Mainly, because it's based on assumptions how they are ...
Aug. 6, 2014, 2:22 p.m. (2014-08-06 14:22:20 UTC) #8
Oleksandr
Oct. 7, 2014, 10:01 p.m. (2014-10-07 22:01:41 UTC) #9
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.)

Powered by Google App Engine
This is Rietveld