Skip to content

Added IDxcVersionInfo3 for custom version string#3517

Merged
adam-yang merged 4 commits intomicrosoft:masterfrom
adam-yang:version_info-3
Mar 2, 2021
Merged

Added IDxcVersionInfo3 for custom version string#3517
adam-yang merged 4 commits intomicrosoft:masterfrom
adam-yang:version_info-3

Conversation

@adam-yang
Copy link
Copy Markdown
Contributor

No description provided.

@adam-yang adam-yang requested a review from tex3d February 26, 2021 22:58
@AppVeyorBot
Copy link
Copy Markdown

Comment thread include/dxc/dxcapi.h Outdated

CROSS_PLATFORM_UUIDOF(IDxcVersionInfo3, "5e13e843-9d25-473c-9ad2-03b2d0b44b1e")
struct IDxcVersionInfo3 : public IDxcVersionInfo2 {
virtual HRESULT STDMETHODCALLTYPE GetCustomVersionString(_Outptr_result_z_ char **pCommitHash) = 0;
Copy link
Copy Markdown
Contributor

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 something other than pCommitHash?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also we don't communicate the protocol here that this is a CoTaskMemAlloc that must be freed by the user with CoTaskMemFree. I know this is an existing problem with the previous interface method as well. Is there a SAL annotation that can be used to express this? Or at least a comment? We have the same issue with IDxcCompiler2::Compile2(..., ppDebugBlobName, ...), but it says (Must be HeapFree()'d!), which isn't accurate it would seem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed typo, added a comment saying it must be HeapFree()'d

@AppVeyorBot
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

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

LGTM!

@adam-yang adam-yang merged commit a7770e6 into microsoft:master Mar 2, 2021
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

Successfully merging this pull request may close these issues.

4 participants