Support for Safearrays!

[dwcullop]

After reading issue #114 I thought that would be a good place for me to start contributing to WIL. So, I've added two main class templates: one for creating safearrays and one for accessing their data (ranged-for loops anyone?). The safearray class can either be generic where you must supply the type or it can have a specific datatype be baked in, and there is a typedef for all the combinations of types and error policies.

I wanted to add some helper functions that perform the most common tasks, but thought it would be better to do that after getting some feedback on what those should look like.

I added a lot of sample usages and tried to follow the source documentation convention used by the other files.

All feedback is appreciated.

[sylveon]

Maybe also add unique_safearray<T>?

[ChrisGuzak]

this is a large addition for something that won't be used by many so I think it would be best in its own header to avoid slow compiles.

see below, this is from one of my attempts at this in the past

    // These types exist to enable specifying the type of the VARIANT
    // to the constructor overloads of VariantArg and Variant. This is necessary
    // because some Win32 types are not distinguished in the C++ type system. This includes
    // BSTR == PWSTR
    // BOOL == int
    // HRESULT == long
    // VARIANT_BOOL == short
    // DATE == double

    namespace VariantTypes
    {
        // VARIANT only string type is BSTR. Since the compiler cannot distinguish the incompatible
        // PWSTR values callers must explicitly state that they have verified the type
        // by specifying this parameter.
        enum IsBSTR { IsBSTR };

        // these are both long
        enum IsLONG { IsLONG };
        enum IsHRESULT { IsHRESULT };

        // these are both int
        enum IsINT32 { IsINT32 };
        enum IsBOOL { IsBOOL };

        // these are both short
        enum IsINT16 { IsINT16 };
        enum IsVARIANTBOOL { IsVARIANTBOOL };

        // these are both double
        enum IsDATE { IsDATE };
        enum IsDOUBLE { IsDOUBLE };
    }

[dwcullop]

this is a large addition for something that won't be used by many so I think it would be best in its own header to avoid slow compiles.

That's how I had it until just before I submitted the PR. I'll revert that commit because I do like that better.

see below, this is from one of my attempts at this in the past

That's a good approach. This is one of more more trickier aspects of dealing with safearrays. I focused on the user knowing a little bit about the underlying type (e.g., to use BSTR and not LPCWSTR or VARIANT_BOOL and not bool). Doing that only creates an issue for short/BSTR and double/DATE.

For those you'll have to create them specifying the VT type explicitly. However, the safearraydata_t class doesn't care about the types. As long as the underlying size is the same, it will work.

If people want a safearray class that lets them work with another string type, like std::string, or std::wstring, then I could write a layer on top of unique_bstr_safearray_X that adds the implicit conversions. However, a better way might be to add one a support function that will convert a BSTR safearray into the container and string-class of the user's choice.

[dwcullop]

Maybe also add unique_safearray<T>?

There's already unique_safearray which is a no-throw version of the generic class that can handle any SAFEARRAY type (but you provide the type when necessary).

[ChrisGuzak]

can you use winrt::implements here instead to avoid the IUnknown boilerplate? use WINRT_CUSTOM_MODULE_LOCK, get_module_lock to enable global object counting.

[dwcullop]

Not at all familiar with WinRT but am willing to learn. It will just take a little while to make that change.

[dwcullop]

I converted the class to use WinRT for it's COM implementation and then realized that winrt::implements requires C++17 for the features that it uses, in which case, I'd rather keep it as-is.

[dwcullop]

Unless there's no requirement to support pre-C++17 anymore... Then sure.

[asklar]

@jonwis @dunhor @ChrisGuzak are there any blockers to getting this merged?

[dwcullop]

Apologies for the delay. I completely forgot I was working on this. I've resolved the merge conflicts. Please let me know if there's anything else I need to do to complete this.

[magol]

@jonwis @dunhor @ChrisGuzak Any progress?

[dunhor]

Sorry for the delay. I'll take a look through for anything glaring, but will defer to @ChrisGuzak or others more familiar with SAFEARRAYs & their usage.

Also note that the entire repo has been re-formatted. Since all your changes are in new files, that likely won't cause you major merge conflicts, just note that you'll need to run clang-format on the new code.

[dunhor]

This should be removed and changed to just a normal include guard

[dunhor]

What's the reason for this and double to be commented out? Does I2 & R8 not exist? Comments about why they are not present are more helpful than commented out code

[dwcullop]

I2 and R8 are tricky to support because they're the same as other types as far as the compiler is concerned. short is the same as BSTR and double is the same as DATE.

Up at the top Chris is suggesting using some dedicated enums for each type that could be confused so they can be explicit. I'll look into that.

[dunhor]

Again, comments about why these types are not present are more helpful than leaving in commented out code. I see the DATE/double conflict, however BSTR is defined as a pointer and shouldn't conflict with short - did you mean something else?

But now that I know why they are not present, what's the behavior if I try and use double with this code? Does it give me a compilation error? Does it succeed, but silently give me incorrect behavior because values are interpreted incorrectly? Or does it end up working correctly? This is all information that is more useful to have commented in the code than commented out lines like this.

[dunhor]

Shouldn't this be named more to indicate that it's calling "lock"? E.g. LockSafeArray_scope_exit or something?

(I'm also not a fan of including _scope_exit in the name, but if that's an established naming pattern, so be it)

[dwcullop]

I followed the naming convention used in other places, which is to name the methods and the RAII classes they return after the specific action that is performed (with _scope_exit appended).

Examples:

• semaphore_release_scope_exit // Releases the semaphore at scope exit
• event_reset_scope_exit // Invokes Reset Event at scope exit

But you're not wrong that this version is also doing a lock on construction, but at Scope Exit, it unlocks. Maybe split the difference? Name that function with Lock and the RAII class with unlock.

Most people won't see the RAII class name because the usage is typically:

// Lock until the end of scope
auto lock = SafeArrayLock_scope_exit(&sa);

[dunhor]

Name that function with Lock and the RAII class with unlock.

Yeah, that's basically what I was suggesting (with an additional side note that the trailing _scope_exit in the function name is excessive). I see that some functions have it, but some others don't (e.g. wil::CoInitializeEx etc.)

[dunhor]

If it's non-owning, the name should indicate that better. In C++, the names view (non-modifiable) or span (modifiable) are typically used to indicate that

[dwcullop]

It is owning. When SafeArrayAccessData is invoked, it creates a refcount and a pointer that this class owns until SafeArrayUnaccessData is invoked.

I think calling it view or span would be convey the wrong idea about what is going on.

[dunhor]

Sorry, by non-owning, I meant that it doesn't own the SAFEARRAY object (i.e. not responsible for calling SafeArrayDestroy). Alternative naming suggestions would be to include ref, reference, or lock into the data type name. safearraydata doesn't quite convey its ownership or responsibility.

[dunhor]

Does Doxygen handle this correctly? I'd think you'd need another set of ~~~ indicating the start of a new example

[dunhor]

Suggested change

	typedef safearray_unaccess_scope_exit::pointer pointer;
	typedef T value_type;
	typedef value_type* value_pointer;
	typedef const value_type* const_value_pointer;
	typedef value_type& value_reference;
	typedef const value_type& const_value_reference;
	typedef T value_type;
	typedef value_type* pointer;
	typedef const value_type* const_pointer;
	typedef value_type& reference;
	typedef const value_type& const_reference;

Am I missing something? Is there a reason to deviate from the typical naming conventions used in the STL and other libraries? You can replace the uses of pointer below with just SAFEARRAY* - you shouldn't need a typedef for it (nor do you really need to reference its type in such an obscure way via safearray_unaccess_scope_exit::pointer)

[dunhor]

Can these just be static constexpr bools? That would be much simpler to read

[dunhor]

Suggested change

	template<typename T = element_t, typename wistd::enable_if<is_type_not_set<T>::value, int>::type = 0>
	template<typename T = element_t, wistd::enable_if_t<is_type_not_set<T>::value, int> = 0>

Use the alias template throughout. Less text and much simpler to read