CNanoVDB sync with Houdini#2187
CNanoVDB sync with Houdini#2187jmlait wants to merge 4 commits intoAcademySoftwareFoundation:masterfrom
Conversation
Improvements:
- CNanoVDB supports integer (I) grids
- CNanoVDB has setValue methods to write to voxels
- CNanoVDB has getLeaf methods to acquire raw leaf pointers
Bug Fixes:
- static used for local kernel methods in CNanoVDB
- Remove auto-computed reserved sizes in favour of hard coded sizes
in CNanoVDB. While less maintainble, C99 forbids zero sized
arrays so some compilers crashed when encountering this.
- Initialize key value when constructing an accessor, some compilers
entered bad states with this undefined.
Signed-off-by: Jeff Lait <jlait@andorra.sidefx.com>
Signed-off-by: Jeff Lait <jlait@andorra.sidefx.com>
| @@ -609,12 +769,15 @@ inline void | |||
There was a problem hiding this comment.
Do we need static inline void here?
There was a problem hiding this comment.
I believe static and inline have opposite meanings in this context as they refer to linking options?
There was a problem hiding this comment.
I was basically looking at the signature of cnanovdb_readaccessor_computeDirty that adds the keyword static before inline. To the best of my understanding having inline by itself can fail to link in strict C99 and adding static makes sure that each translation unit gets its own internal copy, so there is no missing symbol of multiple definition problem.
I was able to produce a sample of where linking can fail without static with O0 flag. I have helper.c:
#include <stdint.h>
#include <stdbool.h>
#include "../../CNanoVDB.h"
void helper_function(const void *rootdata)
{
cnanovdb_readaccessor acc;
cnanovdb_readaccessor_init(&acc, rootdata);
(void)acc;
}
and main.c:
#include <stdbool.h>
#include "../../CNanoVDB.h"
extern void helper_function(const void *rootdata);
int main(void)
{
cnanovdb_readaccessor acc;
cnanovdb_readaccessor_init(&acc, (const void *)0);
printf("acc key: %d %d %d\n", acc.mKey.mVec[0], acc.mKey.mVec[1], acc.mKey.mVec[2]);
helper_function((const void *)0);
return 0;
}
Then, I compile it with:
gcc -std=c99 -O0 -c main.c -o main.o
gcc -std=c99 -O0 -c helper.c -o helper.o
gcc main.o helper.o -o test
The linker error I get is:
/usr/bin/ld: main.o: in function `main':
main.c:(.text+0x3b4e): undefined reference to `cnanovdb_readaccessor_init'
/usr/bin/ld: helper.o: in function `helper_function':
helper.c:(.text+0x3b54): undefined reference to `cnanovdb_readaccessor_init'
collect2: error: ld returned 1 exit status`
However, it's not a problem if either I compile it with O1 flag or adding a static keyword just like cnanovdb_readaccessor_computeDirty.
Since it's not a problem with O1 flag, I'm approving the PR as is.
There was a problem hiding this comment.
Oh, I see, I added the static to the files below!
My understanding was inline would suppress symbol creation so stop it from linking as there won't be a symbol; but it shouldn't matter as all callers in the same translation unit should have inlined it...
I'm not sure why I made it static inline elsewhere, to be honest. I've checked the revision locally and sadly the change didn't comment why static was added to the inline methods. But they should be consistent in any rate, and your example shows why "static inline" is a meaningful setup. So I'll add static here.
There was a problem hiding this comment.
Thank you for adding this.
Fix int/uint mismatch with nanovdb Fix mFlags mismatch with nanovdb. Signed-off-by: Jeff Lait <jlait@andorra.sidefx.com>
| @@ -609,12 +769,15 @@ inline void | |||
There was a problem hiding this comment.
I was basically looking at the signature of cnanovdb_readaccessor_computeDirty that adds the keyword static before inline. To the best of my understanding having inline by itself can fail to link in strict C99 and adding static makes sure that each translation unit gets its own internal copy, so there is no missing symbol of multiple definition problem.
I was able to produce a sample of where linking can fail without static with O0 flag. I have helper.c:
#include <stdint.h>
#include <stdbool.h>
#include "../../CNanoVDB.h"
void helper_function(const void *rootdata)
{
cnanovdb_readaccessor acc;
cnanovdb_readaccessor_init(&acc, rootdata);
(void)acc;
}
and main.c:
#include <stdbool.h>
#include "../../CNanoVDB.h"
extern void helper_function(const void *rootdata);
int main(void)
{
cnanovdb_readaccessor acc;
cnanovdb_readaccessor_init(&acc, (const void *)0);
printf("acc key: %d %d %d\n", acc.mKey.mVec[0], acc.mKey.mVec[1], acc.mKey.mVec[2]);
helper_function((const void *)0);
return 0;
}
Then, I compile it with:
gcc -std=c99 -O0 -c main.c -o main.o
gcc -std=c99 -O0 -c helper.c -o helper.o
gcc main.o helper.o -o test
The linker error I get is:
/usr/bin/ld: main.o: in function `main':
main.c:(.text+0x3b4e): undefined reference to `cnanovdb_readaccessor_init'
/usr/bin/ld: helper.o: in function `helper_function':
helper.c:(.text+0x3b54): undefined reference to `cnanovdb_readaccessor_init'
collect2: error: ld returned 1 exit status`
However, it's not a problem if either I compile it with O1 flag or adding a static keyword just like cnanovdb_readaccessor_computeDirty.
Since it's not a problem with O1 flag, I'm approving the PR as is.
|
Thank you for addressing all the comments. Approved. |
cnanovdb_readaccessor linkage settings. Signed-off-by: Jeff Lait <jlait@andorra.sidefx.com>
|
LGTM! |
Improvements:
Bug Fixes: