[PATCH] D19901: [ProfileData] (llvm) Use Error in InstrProf and Coverage

[Vedant Kumar via llvm-commits]

vsk added a comment.

I'll make the suggested fixes to EnumBasedError.

Based on Lang's feedback it seems like this class should live in ProfileData for now, on a trial basis. We could easily revisit this decision once we know more about whether or not each enum value kind should get its own ErrorInfoBase.


================
Comment at: include/llvm/ProfileData/InstrProf.h:303-321
@@ -282,5 +302,21 @@
 
-inline std::error_code make_error_code(instrprof_error E) {
-  return std::error_code(static_cast<int>(E), instrprof_category());
-}
+class InstrProfError : public EnumBasedError<instrprof_error, InstrProfError> {
+public:
+  InstrProfError(instrprof_error Err)
+      : EnumBasedError<instrprof_error, InstrProfError>(Err) {}
+
+  void log(raw_ostream &OS) const override;
+
+  std::string message() const override;
+
+  /// Consume an Error and return the raw enum value contained within it. The
+  /// Error must either be a success value, or contain a single InstrProfError.
+  static instrprof_error take(Error E) {
+    auto Err = instrprof_error::success;
+    handleAllErrors(std::move(E), [&Err](const InstrProfError &IPE) {
+      assert(Err == instrprof_error::success && "Multiple errors encountered");
+      Err = IPE.get();
+    });
+    return Err;
+  }
 
----------------
lhames wrote:
> InstrProfError (and CoverageMappingError) need their own ID members. If they don't have them the Error RTTI will treat them both as EnumBasedErrors.
Yikes! Is this true even if I define distinct ID's for them? Specifically:

```
template <> char EnumBasedError<instrprof_error, InstrProfError>::ID = 0; // InstrProf.cpp
template <> char EnumBasedError<coveragemap_error, CoverageMapError>::ID = 0; // CoverageMapping.cpp
```

================
Comment at: include/llvm/ProfileData/InstrProf.h:602
@@ -561,3 +601,3 @@
   // Scale merged value counts by \p Weight.
-  instrprof_error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
-                                     uint64_t Weight);
+  Error mergeValueProfData(uint32_t ValueKind, InstrProfRecord &Src,
+                           uint64_t Weight);
----------------
In the event of a large number of overflow errors, the malloc traffic out of this API might be a real performance concern. The same goes for all clients of `InstrProfError::merge`.

For now, I will re-work this to return a raw `instrprof_error`. In the future, we may want to change the API again to get the best of both worlds: undroppable errors and no memory overhead.


http://reviews.llvm.org/D19901