One thing I don't understand is why VArrayImpl<bool> & is used here instead of const VArray<bool> &.
It's similar in other places. I think it's to reduce the overhead of going through VArray first for every element?

If so, that information could become a comment somewhere.

I would prefer to keep using blender;:fn::GVArray; here, same with the other spline files, unless there is a good reason not to.

Unnecessary change to comment formatting.

This ought to have a comment explaining its purpose, it's not as clear like the other methods otherwise.

Add a comment explaining why this exists. (My guess is to enable compiler optimizations?)

Could you move these to the bottom of the file in a separate commit? It would be much easier to see the changes that way.

Add a comment explaining this assert and/or a comment explaining how this function is meant to be used.

What about adding more of the functions like materialize and others directly to VArray? Do you think that's reasonable? That way the Impl types wouldn't have to be used as much in external code.

The use of Any here and the method for storing arbitrary virtual array types really deserves some explanation. It's very abstract, and takes a long time for someone not very familiar with this sort of C++ code to understand without that.

This can be a separate change, but I think we ought to rethink the use of ResourceScope here now. It probably makes more sense to allow the function to optionally request a buffer from the evaluator, which the evaluator would know how to reuse later on in the process. But we can think more about that later I think.

It may reduce overhead (still have to look at the optimized assembly).
The main reason however is that I didn't update it yet. VArray was changed to VArrayImpl here using a global find-replace operation.

Will add a comment (didn't manage to finish everything last weekend).
The reason is actually that I don't want people to subclass this, because then the has_ownership_impl below may become wrong (e.g. if the subclass owns some data, which is common).

Could move them down in a separate commit before (the need to be at the bottom, because they use on VArray). Generally, I'm not sure if it is feasible to separate these kinds of changes from the refactor, would make it much harder in some cases. If you think there is a big benefit, then I'll do it though.

Thought about that, but currently I don't think we should do it. Mainly because it would just add more duplicated code.
Although, at a second thought, maybe I can remove from from VArray, so that VArray only contains the virtual methods. Will think about it.

I agree. Can be done afterwards.

That's fine if it's a pain to do. I don't think the benefit is that big.

Your second thought sounds like a good idea!

Any() = default` MSVC triggers a warning for unitialized buffer_ for this default constructor (even though it doesn't matter for the usecase of any since it's just a buffer), but MSVC doesn't warn if you in-class default initialize: AlignedBuffer ... buffer_{}

https://docs.microsoft.com/en-us/cpp/code-quality/c26495?f1url=%3FappId%3DDev16IDEF1%26l%3DEN-US%26k%3Dk(C26495)%26rd%3Dtrue&view=msvc-160

Get a reference ... ?

Get a reference...?

Up to you of course, but I typically find conversions to bool like this harder to read especially if you're not immediately aware of the object's type when you're reading the code. Something like has_value seems like a more illustrative way for the same intent.

if (a) { ... }

vs

if (a.has_value()) { ... }

does not compile for me on MSVC 2019.

Paste of errors: https://developer.blender.org/P2560 (nevermind the pointcloud.cc file stuff--I was too lazy to compile w/ tests so I just copy and pasted the meat of the functions to this file)

Might just be for testing purposes, but I think having developers use the std::in_place_type constructor directly should be avoided when possible. Instead, it'd be easier to use and read make_any which does this under the hood, and many non-expert developers are familiar with the make_XXX API through std::make_unique, std::make_optional, etc... rather than the in_place_type tag versions.

template<typename T, typename... TArgs> auto make_any(TArgs &&...args)
{
  return blender::Any(std::in_place_type<T>, std::forward<TArgs>(args)...);
}

// You can remove the auto if you want but MSVC seems to have a hard time trying to deduce its template arguments. See my other comment.

so instead of

z = Any(std::in_place_type<Any<>>, a);

it becomes

z = make_any<Any<>>(a);

I'm still a bit ambivalent when it comes to that currently. I changed it in a couple of places now, but probably won't change it in more places unless we define a general guideline for Blender at some point.

Generally makes sense. I'll probably not add this for now because it's not really necessary and there are some complications that I'd rather solve when there is a real need.

• What should the name of the new function be. Using the same make_any name may be confusing, even when it is in the blender namespace. Alternatives are make_Any or Any<>::Make, ...
• There is not just one type of Any. So make_any would have to get the template parameters of Any as well.