abedra / libvault

A C++ library for Hashicorp Vault
MIT License
34 stars 25 forks source link

Is TinyString and TinyLong needed? #50

Closed jimpark closed 4 years ago

jimpark commented 4 years ago

Especially with TinyString, I find that the usage of the API become cumbersome:

auto cb = Vault::ConfigBuilder()
  .withDebug(true)
  .withHost(Vault::TinyString{host})
  .withTlsEnabled(true)
  .withPort(Vault::TinyString{"443"});

I'm not sure what TinyString actually brings to the table. I think it would be nicer if we can write:

auto cb = Vault::ConfigBuilder()
  .withDebug(true)
  .withHost(host)
  .withTlsEnabled(true)
  .withPort(443);

If we want to snowflake individual types for protection, the better way would be to have individual types for Host, Token etc.

struct Host {
  explicit Host(std::string v) : v_(std::move(v)) {}
  std::string v_;
};

...
auto cb = Vault::ConfigBuilder()
  .withDebug(true)
  .withHost(Host{host})
  .withTlsEnabled(true)
  .withPort(443);
abedra commented 4 years ago

Let me start by defining my overall goals and preferences without diving into the implementation. Perhaps that will help guide the conversation.

I prefer tiny types for everything. They disambiguate arguments, and eliminate a class of errors caused by simply putting the wrong argument in the wrong order. Think of a method with the signature

int example(std::string, long, long, std::string, std::string);

This example makes it really easy to put the wrong string in the wrong place, and the wrong long in the wrong place. An IDE may help by showing you the name of the argument, but nothing stops the program from compiling if you put string and longs in the required slots.

Additionally, I use tiny types to help define a lexicon for a program or library, such that the domain can be expressed more richly through the types. By doing this I believe more context is shared in a program and the author can more clearly communicate intent.

As for the implementation, it is admittedly lacking in a few places. That isn't too difficult to fix, it just requires a little more template magic. In terms of the lift required to use, I understand the trade off, but prefer it.

Last, Your example above isn't quite accurate. The TinyString class is only used to define the other tiny types. It's a direct substitute, but not my goal. Ultimately, your reference should be

auto cb = Vault::ConfigBuilder()
  .withDebug(true)
  .withHost(Vault::Host{host})
  .withTlsEnabled(true)
  .withPort(Vault::Port{"443"});

Hopefully this helps explain my perspective on this. I'm definitely open to updates on the implementation of tiny types which arguably should be improved, but I do want to preserve the intent of my explanation above.

jimpark commented 4 years ago

I think that's a worthy goal. I strive to do the same myself.

As you can see, the typedefs do not create different types but rather type aliases to the same type. So in the case above, you can just as well write:

auto cb = Vault::ConfigBuilder()
  .withDebug(true)
  .withHost(Vault::Port{host})
  .withTlsEnabled(true)
  .withPort(Vault::Host{"443"});

What you want is to create actual different types that have an underlying storage of type string.

Something like this would do it:

struct Host {
  explicit Host(std::string v) : v_(std::move(v)) {}
  std::string v_;
};

However, if you want more shared functionality but still generate unique types, you can use a template like this.

tinystring.cpp:

#include <string>

enum class Type {
  Host,
  Port
  // ...
};

template <Type T>
class TinyString {
public:
  TinyString() = default;
  virtual ~TinyString() {}

  TinyString(std::string value) : value_(std::move(value)) {}

  friend std::ostream &operator<<(std::ostream &os, const TinyString &object) { return os << object.value(); }
  friend std::string operator+(const std::string &string, const TinyString &tiny) { return string + tiny.value(); }
  friend std::string operator+(const TinyString &tiny, const std::string &string) { return tiny.value() + string; }
  friend std::string operator+(const char *string, const TinyString &tiny) { return string + tiny.value(); }
  friend std::string operator+(const TinyString &tiny, const char *string) { return tiny.value() + string; }
  friend std::string operator+(const TinyString &tiny, const TinyString &other) { return tiny.value() + tiny.value(); }

  [[nodiscard]] bool empty() const { return value_.empty(); }
  [[nodiscard]] const char *c_str() const { return value_.c_str(); }
  [[nodiscard]] const std::string &value() const { return value_; }

protected:
  std::string value_;
};

using Host = TinyString<Type::Host>;
using Port = TinyString<Type::Port>;
// ...

int main() {
  Host h1{"google.com"};
  Host h2{"yahoo.com"};
  h2 = h1; // Should be okay

  Port p{"8080"};
  p = h1; // Error! Different types.
}

The result of compiling this using C++17 with clang is the following:

> clang++ -std=c++17 tinystring.cpp
tinystring.cpp:42:5: error: no viable overloaded '='
  p = h1; // Error! Different types.
  ~ ^ ~~
tinystring.cpp:10:7: note: candidate function (the implicit copy assignment operator) not viable: no known conversion
      from 'TinyString<Type::Host aka 0>' to 'const TinyString<1>' for 1st argument
class TinyString {
      ^
1 error generated.
abedra commented 4 years ago

Agreed. The typedef doesn't truly enforce things and was mostly a placeholder until I could get around to doing it properly. I need to throw in the hash equality stuff as well. There are also a few little things macro wise that can be done to make declaring/using them nicer as well.

jimpark commented 4 years ago

Since these API are mostly thin wrappers to libcurl, I don't think there's a need for having complicated types. The usefulness is really in the snowflaking of the types so that it protects the user from permuting them in function calls. But I don't think you'd want the user to hold these types in their own code.

For example, I wouldn't use Vault::Url in my code. I have my own uri class that can parse URI strings and break them down to protocol, port, host, path, parameters etc. And I internally pass that around in my code. Only when I have to use Vault do I need to convert to the Vault::Url type. So I don't need a complicated type on the Vault side. I wouldn't use Vault::Url in an associative container, so I don't really need the hash specialization or comparison operators.

abedra commented 4 years ago

@jimpark I think #51 is a reasonable compromise. It makes the types real and enforced. I didn't bother with the hash function in the macro because I really don't see it as necessary. If that changes I can add it.