Issue #276 - introduce class BSTR_Argument

src/plugin/COM_Value.cpp

Index: src/plugin/COM_Value.cpp
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/plugin/COM_Value.cpp
@@ -0,0 +1,64 @@
+/**
+ * \file COM_Value.cpp Support for the values used in COM and Automation.
+ */
+#include "COM_Value.h"
+#include <stdexcept>
+
+using namespace AdblockPlus::COM;
+
+/*
+ * MSDN SysAllocStringLen function http://msdn.microsoft.com/en-us/library/windows/desktop/ms221639%28v=vs.85%29.aspx
+ */
+BSTR_Argument::BSTR_Argument( const std::wstring& s )

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

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.

+{
+  auto length = s.length();
+  if ( length == 0 )
+  {
+    /*
+     * BSTR makes no distinction between null pointers and non-null pointers to the empty string.
+     * In order to detect allocation failure reliably, we simply don't allocate empty strings.
+     * Compare with the default constructor.
+     */
+    bstr = nullptr;

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

The comment should be in the header near the ctr, as well as the test for this
ctr is required.

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

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

+    return;
+  }
+  // Assert length >= 1
+  bstr = SysAllocStringLen( s.c_str(), length );
+  /*
+   * Since the string is non-empty,
+   *   if the return value of SysAllocStringLen is null it means that it had an allocation failure.
+   */
+  if ( !bstr )
+  {
+    throw std::bad_alloc();
+  }
+}
+
+/*
+ * MSDN SysFreeString function http://msdn.microsoft.com/en-us/library/windows/desktop/ms221481%28v=vs.85%29.aspx
+ */
+BSTR_Argument::~BSTR_Argument()
+{
+  // There's no need to check for null; SysFreeString already handles that case.
+  SysFreeString( bstr );
+}
+
+BSTR* BSTR_Argument::operator&()
+{
+  // Assign the empty string, if not already empty.

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

I don't like the changing of the value when the address is taken. It's not only
semantically unexpected, but also looks unsafe.
First of all put the assert here, it should not happen. Secondly, return nullptr
or throw an exception.

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

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.
"Looks"? It maintains the life cycle invariants of the underlying value. How is
that unsafe?
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.

+  if ( bstr )
+  {
+    SysFreeString( bstr );
+    bstr = nullptr;
+  }
+  return &bstr;
+}
+
+BSTR_Argument::operator std::wstring() const
+{
+  if ( !bstr )
+  {
+    return std::wstring();
+  }
+  return std::wstring( bstr, SysStringLen( bstr ) );
+}