1

We're playing some code golf at work. The purpose is to keep the signature of to_upper and return all arguments to upper. One of my colleague proposes this ~~ugly~~ brillant code:

#include <iostream>
#include <memory>
#include <stdexcept>
#include <string>

std::string operator+(std::string_view& a, int const& b) {
  std::string res;

  for (auto c : a) {
    res += (c - b);
  }
  return (res);
}

struct Toto {
  std::string data;
};

struct Result {
  std::string a;
  std::string b;
};

std::unique_ptr<Toto> to_upper(std::string_view input_a,
                               std::string_view input_b) {
  auto* res = new Result;
  res->a = (input_a + 32);
  res->b = (input_b + 32);
  auto* void_res = reinterpret_cast<void*>(res);
  auto* toto_res = reinterpret_cast<Toto*>(void_res);

  return std::unique_ptr<Toto>(toto_res);
}

int main() {
  std::unique_ptr<Toto> unique_toto_res = to_upper("pizza", "ananas");

  auto* toto_res = unique_toto_res.release();
  auto* res = reinterpret_cast<Result*>(toto_res);

  std::cout << res->a << std::endl;
  std::cout << res->b << std::endl;
  return 0;
}

Is this use of reinterpret_cast is fine in terms of portability and UB? We think that it's ok because we just trick the compiler on types, but maybe there's something we missed.

Nicolas
  • 598
  • 3
  • 15
  • 1
    I didn't get why you need these casts here, inheriting `Result` from `Toto` and using `dynamic_cast` should solve all problems without fear of UB. – sklott Apr 30 '21 at 12:55
  • 1
    Avoid magical number: `'A' - 'a'` – Jarod42 Apr 30 '21 at 13:00
  • @sklott Wouldn't such a solution require `Toto` base to be a polymorphic class? (To solve a problem with safety.) – Daniel Langr Apr 30 '21 at 13:02
  • 2
    BTW, `a-z` is not guaranteed to be contiguous (EBCDIC is a counter example). so `'A' - 'a'` is not guaranteed to be equal to `'Z' - 'z'`. – Jarod42 Apr 30 '21 at 13:04
  • To start off with, a `reinterpret_cast` to and from `void*` basically never makes sense — [use `static_cast` instead](https://stackoverflow.com/a/310489/1968). – Konrad Rudolph Apr 30 '21 at 13:13
  • @DanielLangr If you mean that `Toto` will need virtual destructor, then answer is: best practice say that it should, but it doesn't HAVE to have it. – sklott Apr 30 '21 at 13:16
  • @sklott To explain a bit further, it technically doesn't need it because the example never deletes `Result` through a pointer to `ToTo`. If it did, **then** the destructor would need to be virtual - or else the delete would have undefined behaviour. – eerorika Apr 30 '21 at 13:17
  • @sklott Not only, the rules for resolving a `dynamic_cast` expression are different when polymorphic and non-polymorphic types are involved. BTW, I suppose the OP may not change the `Toto` class. Otherwise, they would simply add another `string` member variable into it. – Daniel Langr Apr 30 '21 at 13:40
  • You are right about not changing `Toto` @DanielLangr. – Nicolas Apr 30 '21 at 14:03

2 Answers2

2
std::string operator+(std::string_view& a, int const& b)

It might not be exactly disallowed, but defining an operator overload for a standard class in the global namespace is just asking for ODR violations. If you use any libraries and if everyone else thinks this will just be fine, then someone else may also define that overload. So, this is a bad idea.

auto* void_res = reinterpret_cast<void*>(res);

This is entirely unnecessary. You get exactly the same result by reinterpret casting directly to Toto*.

Valid (and portable)

Assuming that lower and upper case are 32 apart isn't an assumption that is portable to all character encodigs. The function also doesn't work as one might expect for characters outside the range of a...z.

Now about the main question. reinterpret_cast a pointer (or reference) to another itself never has UB. It's all about how you use the resulting pointer (or reference).

The example is a bit precarious while the unique pointer owns the reinterpreted pointer because if an exception is thrown, then it would attempt to delete it which would result in UB. But I don't think an exception can be thrown, so it should be OK. Otherwise, you just reinterpret cast back, which is explicitly well defined by the standard in the case the alignment requirement of the intermediate type isn't stricter than the original (which applies to this example).

The program does leak memory.

eerorika
  • 232,697
  • 12
  • 197
  • 326
1

The only problem here is you have a memory leak. You never delete the pointer after you call release.

You are allowed to use reinterpret_cast to cast an object to an unrelated type. You are just not allowed to access that unrelated type. Going from Result* to Toto* and then back to Result* is okay, and you only access the Result object through a Result*.

When doing T* to U* and then back to T* both T and U need to be object types and U cannot have a stricter alignment then T. In this case both Result and Toto have the same alignment so you are okay. This is detailed in [expr.reinterpret.cast]/7

NathanOliver
  • 171,901
  • 28
  • 288
  • 402