[PATCH] D50807: [Error] Add FileError and DebugErrorInfo helpers

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: include/llvm/Support/Error.h:1217-1218
+///
+template <typename ThisErrT, typename EC, typename ECat>
+class DebugErrorInfo : public ErrorInfo<ThisErrT> {
+public:
----------------
zturner wrote:
> I wonder if this should have a more general name.  There's nothing really specific to Debug Info in this implementation, it's basically just a wrapper around a `std::error_code` and `std::string`
Will do. This new `DebugErrorInfo` class ressembles `StringError`, however the behavior for printing the message is different.

- `StringError` only prints the message
- `DebugErrorInfo` prints the message if the `error_code` is `unspecified`; otherwise it prints the associated `error_code` message, along with the user-provided message.
- Similarily, `ECError` is in the same problem domain as the two above, except that it always prints its associated `error_code` message.

I'll see if I can work out a more elegant solution, without adding a new class.


================
Comment at: include/llvm/Support/Error.h:1248
+  std::string ErrMsg;
+  EC Code;
+};
----------------
zturner wrote:
> I wonder if we can get away with removing the `EC` template argument.  A `std::error_condition` is essentially a wrapper around an error code and an error category, so ultimately when we call back into the category to format we're losing the info about the error code type anyway.  It doesn't matter what the enum type is, the only thing that matters is that we know which class can format it.
> 
> So I think we can just make this an `int`.
> 
> Furthermore, I wonder if we need the `ManagedStatic` at all.  All of these categories are cheap to construct, and I think they are all default constructible anyway, so perhaps our `convertToErrorCode()` function could just look like:
> 
> ```
> return std::error_code(Code, ECat{});
> ```
> 
> In all existing use cases, we never pass the same category with a different error code type, so we already havea  1:1 mapping anyway, and I don't think this is too restrictive.  What do you think?
Agreed, see above comment.

As for `convertToErrorCode()`, category objects //cannot //be created on the fly, the reference must be pointing to a singleton, as stated [[ https://en.cppreference.com/w/cpp/error/error_category | here ]] and [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0824r1.html | here ]].

The following shows how `std::error_category` **shouldn't be used**:

```
enum class WrongCatError {
  TestError = 1,
};

class WrongErrorCategory final : public std::error_category {
public:
  const char *name() const noexcept override { return "wrong"; }
  std::string message(int condition) const override { return ""; }
};

} // namespace

namespace std {
template <> struct is_error_code_enum<WrongCatError> : std::true_type {};
} // namespace std

namespace {

std::error_code make_error_code(WrongCatError Error) {
  auto E = std::error_code(static_cast<int>(Error), WrongErrorCategory()); // category is a temporary
  return E;
}

std::error_code SubFn() {
  return WrongCatError::TestError; // implicit call to make_error_code in
                                   // std::error_code's constructor
}

std::error_code SubFn2() {
  int a[200] = {0};
  return SubFn();
}

TEST(Error, WrongCategoryUsage) {
  auto E1 = SubFn();
  auto E2 = SubFn2();
  EXPECT_NE(E1, E2); // we fail because WrongErrorCategory is passed as a temporary
}
```

An example of such misuse can be found in `clang/trunk/lib/Frontend/PrecompiledPreamble.cpp`:
```
std::error_code clang::make_error_code(BuildPreambleError Error) {
  return std::error_code(static_cast<int>(Error), BuildPreambleErrorCategory());
}
```

The standard says that `std::error_category` shall compare **by pointer**, which of course wouldn't work in this case. The `std::error_code` object ends up here with a dangling pointer (at least in my VS2017 15.8 implementation of STL).

Unfortunately, the issue comes from the API to `std::error_code` which is misleading and cannot express the singleton storage requirement:
```
class error_code {
...
error_code( int ec, const error_category& ecat ); // 'ecat' is a reference to an error_category that we won't modify
...
}
```
...when in reality the API contract should say something more like:
```
class error_code {
...
error_code( int ec, const error_category static& ecat ); // 'ecat' would be a reference to an static error_category that we won't modify
...
}
```
...which we cannot write evidently.

This issue is vaguely described in [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0824r1.html | P0824R1 ]].



Repository:
  rL LLVM

https://reviews.llvm.org/D50807