Skip to content

Issue fixing - #87

Merged
YaaZ merged 2 commits into
YaaZ:masterfrom
GrinlexGH:issue-fixing
Jul 27, 2026
Merged

Issue fixing#87
YaaZ merged 2 commits into
YaaZ:masterfrom
GrinlexGH:issue-fixing

Conversation

@GrinlexGH

@GrinlexGH GrinlexGH commented Jun 21, 2026

Copy link
Copy Markdown

@GrinlexGH
GrinlexGH marked this pull request as draft June 22, 2026 18:03
@GrinlexGH

GrinlexGH commented Jun 22, 2026

Copy link
Copy Markdown
Author

Im not sure about private section of the module
Project compiles without it on clang mingw and msvc 18.7.0 insiders preview, so I dont know if it necessary to have
But having this private section blocks the use of gcc because it does not support them, so i think it is better to wrap private section to #ifdef

@GrinlexGH
GrinlexGH marked this pull request as ready for review June 22, 2026 19:44
@sharadhr

Copy link
Copy Markdown

A small nitpick: would it be useful to split this PR up into individual PRs that each addresses one of the issues quoted? It might be easier for review as well.

@GrinlexGH

Copy link
Copy Markdown
Author

I see the point of moving only the addition of abi breaking style to a separate pr, because everything else is a minor change, literally three small fixes

@sharadhr

Copy link
Copy Markdown

Any chance of having this and #88 verified and merged?

@GrinlexGH

Copy link
Copy Markdown
Author

@YaaZ

@YaaZ

YaaZ commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Sorry guys, I was really busy in the last couple months, gonna take a look at this soon. I guess there's no better way to make Volk work with modules other than that conditional include? Also, don't we need to support a scenario when Volk is built as a separate module?

@YaaZ

YaaZ commented Jul 27, 2026

Copy link
Copy Markdown
Owner

There is a CI failure: https://github.com/YaaZ/VulkanMemoryAllocator-Hpp/actions/runs/28969139805/job/89922446044?pr=87
There were new functions in 3.4.0, like vmaAllocateDedicatedMemory, so you need to run a generator and include changes to the generated files. But I suggest doing that version update in a separate PR and leaving only those small fixes there

@GrinlexGH

Copy link
Copy Markdown
Author

Sorry guys, I was really busy in the last couple months, gonna take a look at this soon. I guess there's no better way to make Volk work with modules other than that conditional include? Also, don't we need to support a scenario when Volk is built as a separate module?

Modules are isolated, so the only correct solution is to #include <volk.h> inside the module

As I know, volk is C-only library, so I don't think they will make C++ module

Strangely, I was sure that I had started the generators

@YaaZ YaaZ left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, thanks!

@YaaZ
YaaZ merged commit 1a2a194 into YaaZ:master Jul 27, 2026
26 checks passed
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.

3 participants