Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in ITypeLib 64 bit + Discussion on style of declarations #11

Open
Greedquest opened this issue Feb 11, 2023 · 3 comments
Open

Bug in ITypeLib 64 bit + Discussion on style of declarations #11

Greedquest opened this issue Feb 11, 2023 · 3 comments

Comments

@Greedquest
Copy link

Greedquest commented Feb 11, 2023

https://github.com/fafalone/tbShellLib/blob/93346dece996ba12da8526ad21a923bdb95cc00f/Export/Sources/slTypelib.twin#L33

This should return LongPtr I believe (it is PVOID)


Also I wonder how you would feel about 2 (or 3) improvements to make this more idiomatic:

https://github.com/fafalone/tbShellLib/blob/93346dece996ba12da8526ad21a923bdb95cc00f/Export/Sources/slTypelib.twin#L30

In my code, I have declared this as

Sub GetDocumentation(ByVal memid As MEMID, Optional ByRef pBstrName As String, Optional ByRef pBstrDocString As String = vbNullString, Optional ByRef pdwHelpContext As Long, Optional ByRef pBstrHelpFile As String)

with 2 changes: The Optional arguments, since these are optional from the API point of view. You can specify default of =vbNullString and =NULL_PTR or =0 if needed

Second change is I do

Public Enum MEMID
	[_]
End Enum

This does a sort of typedef for DWORD types which improves readability IMO

3rd change is I rename parameters to for example

Sub GetDocumentation(ByVal memid As MEMID, Optional ByRef outName As String, ...)

Since ByRef pBstrName to me is

  • Too much information; I don't need to know about Bstr as a VBA developer, just String
  • ByRef and p both mean kinda the same thing, doubling up makes me expect to get a pointer to a string out, not just a string
  • I think the outParam prefix is good from a VBA consumer point of view to make it clear. Attributes may be better in future tB.

I think this may be a harder sell as it is backwards incompatible for your library interface. However in my experience when I declare the APIs this way it makes me much less likely to need to look at the MSDN documentation. But it's still easy to look up online even if I've renamed the params, the function name is the same and the order of params is the same. Maybe for version 3.x of your package.

@fafalone
Copy link
Owner

There's planned support for actual aliases. When that arrives, I'm planning to go through and put everything back to it's correct type. It would've been a good idea to use the enum trick from the start (and I use it for LongPtr in VB6 projects, just didn't think of it here)... but might as well wait now. Though I'm confused here-- BSTR is too much info, MEMID (which doesn't even tell you the underlying type) is needed info? Which way do we want to go ;)

Normally I omit ByRefs as is standard for VB, but for a lot of the project, I started with the output of tB's typelib viewer which has them. I do agree it would make sense to go through and remove them from all for consistency. But as far as renaming parameters... this project started as Edanmo's olelib, so at this point there's over two decades of usage of backwards compatibility to consider. I'm very reluctant to break that outside of bugs, even when I think the alternative is much better (like matching the documentation for whether the last out param is a retval). But even for new interfaces... I don't these are the types of things you just play around with and find out... to use them you'll be looking up documentation and examples, and I'm not sure if creating confusing by renaming parameters from what those will have is worth the added clarity, though if others want to weigh in and consensus is otherwise, we could certainly do that.

Re: Optional... the original problem was MKTYPLIB does not support the optional attribute for interfaces, so they were all removed from oleexp, which I generated this project from. Since tB does it's certainly worth looking at adding them back in... but I'd only consider this where they're optional in the SDK header, and ITypeLib/ITypeInfo::GetDocumentation, they're not listed as optional.

In the mean time, I'll post an update with the bug fix tonight, thanks for letting me know.

@fafalone
Copy link
Owner

Issue with ITypeInfo::AddressOfMember fixed (I believe you meant this one as ITypeLib has no such member, right?) in v2.9.84.

Will leave this open to discuss other things.

@Greedquest
Copy link
Author

Greedquest commented Feb 22, 2023

Though I'm confused here-- BSTR is too much info, MEMID (which doesn't even tell you the underlying type) is needed info? Which way do we want to go ;)

The difference is, bstr doesn't tell you what the variable means but memid does (it tells you it's a member id). Maybe memberID As MEMID would be better from VBA's perspective.

Normally I omit ByRefs as is standard for VB, but for a lot of the project, I started with the output of tB's typelib viewer which has them. I do agree it would make sense to go through and remove them from all for consistency.

Actually I meant the opposite - I'd drop the p prefix which is a convention and instead specify ByRef which is a language keyword. Language features are always preferable to naming conventions IMHO. E.g. I suggest naming variables ByRef outParam now, but if tB supports an out attribute for params that I'd definitely prefer that (as long as it's equally clear at the callsite).


RE Optional and renaming params. Sure the SDK may say one thing but VBA is a different language to C and therefore I think the best way to represent these features is using Optional. I think the behaviour is indistinguishable right?

Just depends whether you want to focus on fidelity or usability. To me usability is generally the motivation. I understand any backwards incompatible change would necessitate a major version bump to notify users, and you may risk losing some users who for whatever reason prefer the old style, but that is a tradeoff for more new users who may prefer the new approach. Hard call to make.

I can just confirm one voice saying actually for my use cases, having these interfaces match the aesthetics of the C equivalents - in terms of parameter naming and optional params, is not too important. I think it's a mistake that -people too often transliterate these declarations - rewriting C/C++ code in VBA, rather than translating them into idiomatic VBA. As long as the behaviour is well represented I'm okay with some creative license. But I understand this is highly subjective and maybe not aligned with your motivations for creating this library, or some use case I have overlooked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants