-
Notifications
You must be signed in to change notification settings - Fork 51
Feature completion for C++ drjit::Local #442
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
FormattingWhat settings for clang-format are you using? Unfortunately there is no NamingPlease comment on the naming of functions and variables. I tried to keep it consistent with existing drjit nomenclature, but might have diverged accidentally. This code has seen a few iterations so there might be organically-grown inconsistency. |
|
Hello @diiigle, the formatting looks fine. The codebase usually skips opening/closing braces for conditionals/loops if the body is a simple 1-liner. Please let us know once you've tested |
|
Hi @wjakob, Thanks for the feedback. I adjusted the formatting in 2559c66. Struct support is now fully tested on JIT and scalar variants with the addition of 48f58ee. I had to add the Index type as a template parameter because I couldn't figure out how to access the underlying array type from a Everything can be squashed before merging |
|
Two more minor requests: can you move the lambda functions out into top-level (template) functions that take First: Some compilers (MSVC cough) are not very good about compiling lambda functions, and nesting them two levels deep is just asking for trouble. Second: Both files have whitespace errors (trailing whitespace). I usually set my editor to mark those in red so that it's easily noticable. |
|
Yeah I had been following the ideas in this post to get the neatest assembly out of nested lambdas, but I understand a normal templated function should be much simpler to compile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more minor things, then this is ready to go in.
|
Squashed any temporary commits and rebased onto master. Ready to merge from my side. |
include/drjit/local.h
Outdated
| } | ||
|
|
||
| ~Local() = default; | ||
| Local(const Local &) = delete; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjakob What is the reasoning for disallowing copies? I assume it's performance motivated?
That prevents the use of dr::Local as a member in a DRJIT_STRUCT higher up, as they require
Name &operator=(const Name &) = default; \Meaning it can be wrapped in a DRJIT_STRUCT itself
Closes #426
Adding multiple features to the C++ version of drjit::Local:
DRJIT_STRUCTdata structuresAll while retaining the stack-allocated scalar variant.
TODO before merging:
DRJIT_STRUCTsupport