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

Issue 5698769010032640: Issue #1466 - fix usage of BSTR (Closed)

Created:
Oct. 8, 2014, 2:48 p.m. by sergei
Modified:
Feb. 5, 2015, 10:35 a.m.
Visibility:
Public.

Description

fix calls where `BSTR` is expected but `wchar_t*` was used

Patch Set 1 #

Total comments: 8

Patch Set 2 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -43 lines) Patch
M src/plugin/AdblockPlusDomTraverser.cpp View 1 2 chunks +18 lines, -18 lines 0 comments Download
M src/plugin/PluginDomTraverserBase.h View 1 9 chunks +25 lines, -25 lines 0 comments Download

Messages

Total messages: 13
sergei
Oct. 8, 2014, 3:07 p.m. (2014-10-08 15:07:33 UTC) #1
Eric
http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp File src/plugin/AdblockPlusDomTraverser.cpp (right): http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp#newcode52 src/plugin/AdblockPlusDomTraverser.cpp:52: if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ...
Oct. 9, 2014, 1:54 p.m. (2014-10-09 13:54:01 UTC) #2
sergei
http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp File src/plugin/AdblockPlusDomTraverser.cpp (right): http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp#newcode52 src/plugin/AdblockPlusDomTraverser.cpp:52: if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ...
Oct. 16, 2014, 8:53 a.m. (2014-10-16 08:53:25 UTC) #3
Oleksandr
http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp File src/plugin/AdblockPlusDomTraverser.cpp (right): http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp#newcode52 src/plugin/AdblockPlusDomTraverser.cpp:52: if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ...
Oct. 17, 2014, 7:38 a.m. (2014-10-17 07:38:02 UTC) #4
Eric
http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp File src/plugin/AdblockPlusDomTraverser.cpp (right): http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp#newcode52 src/plugin/AdblockPlusDomTraverser.cpp:52: if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ...
Oct. 20, 2014, 4:06 a.m. (2014-10-20 04:06:24 UTC) #5
Oleksandr
http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp File src/plugin/AdblockPlusDomTraverser.cpp (right): http://codereview.adblockplus.org/5698769010032640/diff/5629499534213120/src/plugin/AdblockPlusDomTraverser.cpp#newcode52 src/plugin/AdblockPlusDomTraverser.cpp:52: if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ...
Oct. 21, 2014, 11:53 p.m. (2014-10-21 23:53:19 UTC) #6
Oleksandr
LGTM
Jan. 6, 2015, 1:43 p.m. (2015-01-06 13:43:47 UTC) #7
sergei
rebased
Jan. 29, 2015, 5:11 p.m. (2015-01-29 17:11:24 UTC) #8
Eric
LGTM. I really don't like to have to allocate BSTR each time for what could ...
Feb. 2, 2015, 3:17 p.m. (2015-02-02 15:17:55 UTC) #9
sergei
On 2015/02/02 15:17:55, Eric wrote: > LGTM. > > I really don't like to have ...
Feb. 2, 2015, 3:22 p.m. (2015-02-02 15:22:03 UTC) #10
Eric
On 2015/02/02 15:22:03, sergei wrote: > Yes, I also don't like it but it's better ...
Feb. 2, 2015, 4:09 p.m. (2015-02-02 16:09:41 UTC) #11
sergei
On 2015/02/02 16:09:41, Eric wrote: > On 2015/02/02 15:22:03, sergei wrote: > > Yes, I ...
Feb. 4, 2015, 11:52 a.m. (2015-02-04 11:52:57 UTC) #12
Eric
Feb. 4, 2015, 12:36 p.m. (2015-02-04 12:36:08 UTC) #13
On 2015/02/04 11:52:57, sergei wrote:
> You propose to prepare at the compile time the
> memory layout for such BSTR strings, thus initialize the structure { const
> uint32_t length; const wchar_t value[];}; at the compile time, don't you? 

Yes, that's exactly the idea.

It should also be possible to do something similar at static initialization
time, to set up the correct memory layout just once, and to do it in a way that
always succeeds and never throws. If that were working, then adding the
declaration 'constexpr' would simply cause it to be initialized at compile time.
The key would be to use a template function that grabs the length of a
wide-character array, and then to use that length as a template argument to
declare the memory-layout structure.

> That looks actually very attractive, feel free to create the ticket for it.

Repeating the allocations with each call is hardly the worst of our
inefficiencies.

And while it looks attractive, there remains one problem with it. There's a
stated requirement (although not stated very explicitly or even frequently) that
all BSTR that goes through COM be allocated with SysAllocString(). Perhaps this
only really means "we require the BSTR memory layout", but it might also mean
"COM needs to interact with the allocator in mysterious ways". It's possible
this requirement arose at the time "for future development" and that future
development never happened. If that's the case, then compile-time initialization
should be fine, because COM is basically finished and done at this point.

The fully correct way of getting the effect of static initialization and also
uses SysAllocString() is to write an explicit initialization routine that runs
when the DLL is loaded. It would need a matching routine that runs when the DLL
is unloaded to deallocate.

Powered by Google App Engine
This is Rietveld