Repository: flutter
Issue: [engine] `FML_DISALLOW_*` is an anti-pattern?
Labels: engine,P2,c: tech-debt,team-engine,triaged-engine

The C++ Flutter engine makes extensive use of [`FML_DISALLOW_*` macros](https://github.com/flutter/engine/blob/ff2f710df4ed554477eceb9d24b9d1b69e9f12b4/fml/macros.h#L18-L42).

These allow writing, for example, instead of:

```h
class Foo {
 public:
   // Disallow copying.
   Foo(const Foo&) = delete;

   // Disallow assigning.
   Foo& operator=(const Foo&) = delete;
}
```

... the following:

```h
class Foo {
 private:
   FML_DISALLOW_COPY_AND_ASSIGN(Foo);
}
```

The [C++ style guide writes](https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types), emphasis mine:

> Every class's public interface must make clear which copy and move operations the class supports. 
>
> This should usually take the form of **explicitly declaring and/or deleting the appropriate operations** in the public section of the declaration.
>
> Specifically, a copyable class should explicitly declare the copy operations, a move-only class should explicitly declare the move operations, and a non-copyable/movable class should explicitly delete the copy operations. A copyable class may also declare move operations in order to support efficient moves. Explicitly declaring or deleting all four copy/move operations is permitted, but not required. If you provide a copy or move assignment operator, you must also provide the corresponding constructor.

I ran into this confusion when I realized that `FML_DISALLOW_COPY` (and `FML_DISALLOW_COPY_AND_ASSIGN`) _also_ (implicitly) disallows moving, which is confusing and a potential source of frustration and/or bugs. Consider the following:

```cc
class NoCopy {
 public:
   NoCopy() = default;

   // This is the same signature as using FML_DISALLOW_COPY. 
   NoCopy(NoCopy&) = delete;
};

TEST(NoCopy, PreventsMove) {
  NoCopy a = NoCopy();

  // ERROR: Candidate constructor not viable: expects an lvalue for 1st argument
  NoCopy b = std::move(a);
}
```

This is because the move constructor was not provided, i.e.:

```cc
class NoCopyAllowMove {
 public:
  NoCopyAllowMove() = default;

  // This is the same signature as using FML_DISALLOW_COPY.
  NoCopyAllowMove(NoCopyAllowMove&) = delete;

  // New (was not in "NoCopy").
  NoCopyAllowMove(NoCopyAllowMove&&) = default;
};

TEST(NoCopyAllowMove, Ok) {
  NoCopyAllowMove a = NoCopyAllowMove();

  // ERROR: Candidate constructor not viable: expects an lvalue for 1st argument
  NoCopyAllowMove b = std::move(a);
}
```

... basically, `FML_DISALLOW_COPY` is misleading, because, by default, it does not permit moving!

(I ran into this with `fml::ScopedCleanupClosure`).

---

Proposal:

1. Stop using the `FML_DISALLOW_*` macros and instead explicitly define and delete constructors.
2. Replace existing `FML_DISALLOW_*` macros with explicit deletions.

For those curious, there is also an awesome primer available here: https://abseil.io/tips/77.

The internal style guide also mentions the following (Googlers can read [go/cstyle#Copyable_Movable_Types](http://go/cstyle#Copyable_Movable_Types):

> You may encounter legacy code that uses the DISALLOW_COPY_AND_ASSIGN macro (from `<omitted>/macros.h`) in the private section to make a class neither copyable nor movable, but **this is no longer permitted in new code.**

This is because these original macros were used to make the constructors _private_, but since the addition of the `delete` operator are now, IMO, just misleading/confusing (see above).