Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Support for Safearrays!#145

Open
dwcullop wants to merge 40 commits intomicrosoft:master from
dwcullop:user/dwcullop/safearrays
Open

Support for Safearrays! #145
dwcullop wants to merge 40 commits intomicrosoft:master from
dwcullop:user/dwcullop/safearrays

Conversation

@dwcullop
Copy link
Member

@dwcullop dwcullop commented Aug 22, 2020 •
edited
Loading

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.

crowbar27 reacted with thumbs up emoji
Darrin Cullop added 19 commits August 3, 2020 14:10 
... will expose functions that require a type or it can be another type and SFINAE will only expose functions assuming that type
Test functions are a mess.
Whiped up some fresh sfinae helpers
...e unique_any types can't have their own members.
...the way that I want. All that is missing is some helper functions to copy/move to/from vectors/arrays to safearrays.
Fixed the safearraydata class
Added function to look up size of a single dimension
Added retrieval of vartype and safer assertions
Moved the Safearray code into Resource.h
Copy link
Contributor

@sylveon sylveon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add unique_safearray<T>?

Copy link
Member

@ChrisGuzak ChrisGuzak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 };
 }

Copy link
Member Author

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.

Darrin Cullop added 4 commits August 22, 2020 12:31 
1) Put safearray code back into it's own header
2) Changed var_traits to use better naming convention and use static constexpr over enum
3) Inline documentation and sample usage
4) Some attempts to make clang happy
Some code clean up / reorg for consistency
Hopefully all the clang errors are fixed
Defined shared/weak versions of SAFEARRAY types
Copy link
Member Author

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).

 

Wow, clang is super picky about unused local typedefs. It's ridiculous.
STDMETHOD(GetID)(LONG* pOut) = 0;
};

class TestComObject : virtual public IAmForTesting,
Copy link
Member

@ChrisGuzak ChrisGuzak Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@dwcullop dwcullop Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dwcullop and others added 3 commits September 14, 2020 10:18 
Co-authored-by: Chris Guzak <chris_guzak@msn.com>
Co-authored-by: Chris Guzak <chris_guzak@msn.com>
Copy link
Member

asklar commented May 31, 2023

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

Copy link
Member Author

dwcullop commented Oct 3, 2023

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.

Copy link

magol commented Feb 21, 2024

@jonwis @dunhor @ChrisGuzak Any progress?

Copy link
Member

dunhor commented Feb 22, 2024

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.
dwcullop reacted with thumbs up emoji

Comment on lines +23 to +24
#if !defined(__WIL_SAFEARRAY_)
#define __WIL_SAFEARRAY_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dwcullop reacted with thumbs up emoji
{
template<typename T> struct infer_safearray_type_traits{};
template<> struct infer_safearray_type_traits<char> { static constexpr auto vartype = VT_I1; };
//template<> struct infer_safearray_type_traits<short> { static constexpr auto vartype = VT_I2; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@dwcullop dwcullop Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


//! Guarantees a SafeArrayUnlock call on the given object when the returned object goes out of scope
//! Note: call SafeArrayUnlock early with the reset() method on the returned object or abort the call with the release() method
WI_NODISCARD inline safearray_unlock_scope_exit SafeArrayUnlock_scope_exit(SAFEARRAY* psa) WI_NOEXCEPT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@dwcullop dwcullop Mar 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

//! Accessing a safearray increases it's lock count, so attempts to destroy the safearray will fail until it is unaccessed.
//! This class works even if the SAFEARRAY is multi-dimensional by treating it as a large single-dimension array,
//! but doesn't support access via a multi-dimensional index.
//! NOTE: This class does not manage the lifetime of the SAFEARRAY itself. See @ref safearray_t.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@dwcullop dwcullop Mar 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

 

Co-authored-by: Duncan Horn <40036384+dunhor@users.noreply.github.com>
//! }
//! return S_OK;
//! }
//! ~~~~
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +229 to +234
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +344 to +358
// SFINAE Helpers to improve readability
template<typename T>
using is_interface_type = wistd::disjunction<wistd::is_same<T, LPUNKNOWN>,
wistd::is_same<T, LPDISPATCH>>;
template<typename T>
using is_pointer_type = wistd::disjunction<wistd::is_same<T, BSTR>,
wistd::is_same<T, LPUNKNOWN>,
wistd::is_same<T, LPDISPATCH>>;

template<typename T>
using is_type_not_set = wistd::is_same<T, void>;
template<typename T>
using is_type_set = wistd::negation<is_type_not_set<T>>;
template <typename T>
using is_value_type = wistd::conjunction<wistd::negation<is_pointer_type<T>>, is_type_set<T>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

explicit safearray_t(args_t&&... args) WI_NOEXCEPT : storage_t(wistd::forward<args_t>(args)...) {}

// Exception-based construction
template<typename T = element_t, typename wistd::enable_if<is_type_not_set<T>::value, int>::type = 0>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template<typename T = element_t, typenamewistd::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

dwcullop and others added 2 commits August 16, 2024 12:46
Copy link

magol commented Apr 11, 2025

@dwcullop Any progress to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunhor dunhor dunhor left review comments

@ChrisGuzak ChrisGuzak Awaiting requested review from ChrisGuzak

+1 more reviewer

@sylveon sylveon sylveon left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments

AltStyle によって変換されたページ (->オリジナル) /