Feature completion for C++ drjit::Local#442
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. |
wjakob
left a comment
There was a problem hiding this comment.
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. |
| @@ -29,15 +39,9 @@ template <typename Value, size_t Size, typename SFINAE = int> struct Local { | |||
|
|
|||
| ~Local() = default; | |||
| Local(const Local &) = delete; | |||
There was a problem hiding this comment.
@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
fc79b70 to
bdc17f6
Compare
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