I'm not a huge fan of creating an object to represent what essentially a null value. I would prefer that instead of passing in a const Sampler& sampler we passed in const std::unique_ptr<Sampler>& sampler. Then we can keep the semantics all the same.

I think IsValid() is an anti-pattern. We shouldn't have invalid objects. It starts polluting your code where you have to check for validity all the time. Or it doesn't and you just have potential bugs everywhere.

We can't use unique_ptr and have a global cache of samplers.

You already are switching to a unique_ptr but you are dereferencing it below. All you have to do is not dereference this and pass the reference to the unique_ptr.

You already are switching to a unique_ptr but you are dereferencing it below. All you have to do is not dereference this and pass the reference to the unique_ptr.

Right here, this is a no-no. You are transforming a std::unique_ptr to a reference. References are non-null, std::unique_ptr are nullable. If you return the reference to the unique_ptr you avoid that class of bugs.

We cannot return a reference to a unique_ptr though?

(from a hash map, specifically)

Yea, you can. Depending on the method you might have to do that trick where you have a static nullptr std::unique_ptr that you return the reference to sometimes.

Here's an example:

#include <map>
#include <memory>

class Foo {};

static const std::unique_ptr<Foo> kNullFoo;

const std::unique_ptr<Foo>& find(const std::map<int, std::unique_ptr<Foo>>& map) {
  const auto& it = map.find(123);
  if (it == map.end()) {
    return kNullFoo;
  } else {
    return it->second;
  }
}
int main() {
  std::map<int, std::unique_ptr<Foo>> map;

  map.insert(std::make_pair(123, std::make_unique<Foo>()));

  const std::unique_ptr<Foo>& value = find(map);

  return 0;
}

Pushed it up

You have to make it return a const std::unique_ptr<const Sampler>& to fix what you have right now. I'm not sure if a const Sampler will work for you. I don't know why that is off the bat

--- a/impeller/renderer/backend/vulkan/sampler_library_vk.cc
+++ b/impeller/renderer/backend/vulkan/sampler_library_vk.cc
@@ -16,9 +16,9 @@ SamplerLibraryVK::SamplerLibraryVK(
 
 SamplerLibraryVK::~SamplerLibraryVK() = default;
 
-static const std::unique_ptr<Sampler> kNullSampler = nullptr;
+static const std::unique_ptr<const Sampler> kNullSampler = nullptr;
 
-static const std::unique_ptr<Sampler>& find_thing(
+static const std::unique_ptr<const Sampler>& find_thing(
     const SamplerMap& map,
     const SamplerDescriptor& desc) {
   const auto& it = map.find(desc);

(since you were using a const Sampler& it should be good)

OH DUH

OKAY! I DID IT

Thanks @gaaclarke I would have never figured that out!

This is potentially a dangling pointer now. What guarantees do we have that this reference will remain valid for the lifetime of the TextureAndSampler?

If we can be confident that it will remain valid, we should add documentation. If we aren't so sure I think maybe we should go back to std::shared_ptr but keep all your work converting the arguments to references. That would eliminate all those shared_ptr copies, except this one.

The platform sampler libraries store the sampler in a unordered_map (which has stable references even when it reallocates) and never evicts them. This should keep the references valid for the lifetime of the program.

This should keep the references valid for the lifetime of the program.

The lifetime of the Context, right? Can you add a comment for this please.

Done: https://github.com/flutter/engine/pull/49974/files#diff-c3cd8ebc7ccac686f1868502bf7b1b710dbf93217b21f34c6ab39c84095cec5aR26

Also I deleted Sampler::isValid :)

I think a comment right here at the field would be appropriate too since looking at this code peoples spider sense tingles for dangling pointers.

This code is almost done being deleted :)