Skip to content

Latest commit

 

History

History
366 lines (278 loc) · 6.47 KB

comment-smells.md

File metadata and controls

366 lines (278 loc) · 6.47 KB

Comment smells


class MyWidget {
  std::mutex mutex_;

public:
  // don't forget to unlock
  void lock();
  // only valid if you locked
  void unlock();

  // must hold lock
  void tinker_with(int amount); 
};

A bad design?


void tinker(MyWidget &widget) {
  widget.lock();
  widget.tinker_with(123);
  widget.tinker_with(333);
  widget.unlock();
}

Improvements!


class MyWidget {
  // ...

  std::scoped_lock<std::mutex> lock();

  // must hold lock
  void tinker_with(int amount); 
};

Spot the mistake?


void tinker(MyWidget &widget) {
  widget.lock();
  widget.tinker_with(123);
  widget.tinker_with(333);
}

Improvements!


class MyWidget {
  // ...

  [[nodiscard]] 
  std::scoped_lock<std::mutex> lock();

  // must hold lock
  void tinker_with(int amount);
};

error: ignoring return value of 
  'scoped_lock<mutex> MyWidget::lock()',
        declared with attribute 'nodiscard'
  |   widget.lock();
  |               ^

Improvements!


void tinker(MyWidget &widget) {
  auto lock = widget.lock();
  widget.tinker_with(123);
  widget.tinker_with(333);
}

Last apology


  // must hold lock
  void tinker_with(int amount);

Mutator interface


class MyWidget {
  void tinker_with(int amount);
public:
  class Tinkerable;

  Tinkerable get_tinkerable() { 
    return Tinkerable(*this); 
  }
};

Mutator interface


class MyWidget::Tinkerable {
  MyWidget &widget_;
  std::scoped_lock<std::mutex> lock_;
  friend MyWidget;

  explicit Tinkerable(MyWidget &widget) 
    : widget_(widget), 
      lock_(widget_.mutex_) {}
    
public:
  void tinker_with(int amount) {
    widget_.tinker_with(amount);
  }
};

Using the Mutator interface


void tinker(MyWidget &widget) {
  auto tinkerable = widget.get_tinkerable();
  tinkerable.tinker_with(123);
  tinkerable.tinker_with(333);
}

Don't call us, we'll call you


class MyWidget {
  std::mutex mutex_;

  class WidgetState {
    int state = 123;
  public:
    void tinker_with(int amount);
  };
  WidgetState state_;

public:
  template <typename TinkerFunc>
  void tinker(TinkerFunc tinker_func);
};

Don't call us, we'll call you


template <typename TinkerFunc>
void MyWidget::tinker(
    TinkerFunc tinker_func) {
  std::scoped_lock lock(mutex_);
  tinker_func(state_);
}

Don't call us, we'll call you


void tinker(MyWidget &widget) {
  widget.tinker(
    [](auto &tinkerable) {
      tinkerable.tinker_with(123);
      tinkerable.tinker_with(333);
    }
  );
}

Other comment smells


class ShaderRegistry {
public:
  void add(const char *shader);

  // once all shaders are added, compile
  void compile();

  // get a compiled shader by name.
  // must be compiled and linked!
  CompiledShader &get_compiled(
    const char *name) const;
};

Separating concerns


class CompiledShaders {
public:
  CompiledShader &get_compiled(
    const char *name) const;
};

class ShaderCompiler {
public:
  void add(const char *shader);

  [[nodiscard]]
  CompiledShaders compile() const;
};

Separating concerns


void use() {
  ShaderCompiler compiler;
  compiler.add("bob");
  compiler.add("dawn");

  auto shaders = compiler.compile();
  shaders.get_compiled("bob").render();
}

Destructive separation


class ShaderCompiler {
public:
  void add(const char *shader);

  // Resources used in compilation are 
  // transferred to the CompiledShaders:
  // you cannot call compile() twice!!
  [[nodiscard]]
  CompiledShaders compile();
};

Destructive separation


class ShaderCompiler {
public:
  void add(const char *shader);

  [[nodiscard]]
  CompiledShaders compile() &&;
};

Destructive separation


auto shaders = compiler.compile();

error: passing 'ShaderCompiler' as 'this' argument
     discards qualifiers [-fpermissive]
  |     auto foo = sc.compile();
  |                           ^
in call to 'CompiledShaders ShaderCompiler::compile() &&'
  |   CompiledShaders compile() &&;
  |                   ^~~~~~~

Destructive separation


auto shaders = 
    std::move(compiler)
    .compile();

Destructive separation


CompiledShaders compile() {
  ShaderCompiler compiler;
  compiler.add("bob");
  compiler.add("dawn");
  return compiler.compile(); // sadly this does not work :'(
}

void use() {
  auto shaders = compile();
  shaders.get_compiled("bob").render();
}

Destructive separation


  auto shaders = 
    std::move(compiler)
    .compile();
  shaders.get_compiled("bob").render();
  // oops
  compiler.add("persephone");

Compiler returned: 0

clang-tidy

warning: 'compiler' used after it was moved
  compiler.add("persephone");
  ^
move occurred here:
  std::move(compiler)
  ^

Summary

  • Protect invariants with code
  • Apologetic comment anti-pattern
    • // Must .. smells
  • Separation of concerns
  • clang-tidy is your friend