-
Notifications
You must be signed in to change notification settings - Fork 403
Marking VmaMalloc and VmaFree as static inline
#514
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
base: master
Are you sure you want to change the base?
Conversation
|
I've pushed the fix as you suggested, on my fork for now: Please let me know if it works for you. |
Instead of `static`, as suggested in GPUOpen-LibrariesAndSDKs#514
|
Thank you for these suggestions. I think you are right -
In the latter case, I agree that using an anonymous namespace is a better solution, considering the implementation of VMA is in C++14. I've changed in. Please let me know if it works for you. |
|
Your fork works for me; tested with Clang 21 and compiled as part of a C++20 module. Anonymous namespace implementation LGTM! Marking some class VmaMappingHysteresis
{
// static const int32_t COUNTER_MIN_EXTRA_MAPPING = 7; // C-style
static constexpr int32_t COUNTER_MIN_EXTRA_MAPPING = 7; // C++-style
};Marking them as |
|
Thanks for this suggestion. I changed some member variables from |
|
Looks good! |
This resolves #513.
Some background first:
When using C++20 modules, exporting VMA leads to issues with the newly released Clang 21. It e.g. complains about
VmaFreebeing an unkown symbol, as it is a TU-local entity with internal linkage (static), which cannot be exported. The solution here is to also mark the function asinline, which allows the compiler to export this function as part of another function that can be exported.Looking through the rest of the code, there are a few redundant or contradictory uses of
inlineandstatic, which I wanted to hear some thoughts about:inlineis redundant, but might be nice as an indication of intent. Some of these are also marked asstatichowever, which would only really make sense for template specializations (of which there aren't any, as far as I could tell).staticin general: This library is intended to be used in both C and C++ environments, correct? I only have experience with the latter, so correct me if it is wrong for either, but do static functions even make sense here (inside the implementation portion)? The header alongsideVMA_IMPLEMENTATIONshould only be used in a single location in the project, so there won't be any ODR violations. Thestatickeyword for this purpose was supposed to be deprecated in C++11 in favor of using anonymous namespaces instead. Since the implementation side is C++, that might be a better solution, if this functionality is even desired.