[PATCH] D51490: [Error] Fix template deduction in createFileError

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: include/llvm/Support/Error.h:1210-1216
+template <class Err, typename = typename std::enable_if<
+                         std::is_base_of<Error, Err>::value &&
+                             !std::is_base_of<ErrorSuccess, Err>::value,
+                         Err>::type>
 inline Error createFileError(std::string F, Err E) {
   return FileError::build(F, std::move(E));
 }
----------------
ilya-biryukov wrote:
> zturner wrote:
> > What if you just write:
> > 
> > `Error createFileError(std::string F, ErrorSuccess) = delete;`
> > 
> > Does that also solve the problem?
> I think this won't work if we want it to work for descendants of ErorrSucces. The reason is that the template would still be preferred for the derived classes, e.g.
> 
> ```
> class MyErrorSuccess : public ErrorSuccess {
>   ///... 
> };
> 
> void test() {
>   createFile("foo", MyErrorSuccess()); // <-- chooses a template, not `=delete`d function.
> }
> ```
> 
> The reason is that the non-template overload needs a derived-to-base conversion for the second arg, so it's considered "worse" than the template one for the purpose of the overloading.
> 
> Haven't checked with the compiler, though.
If using the `= delete` suggestion, `createFileError("file.bin", ErrorSuccess())` fails with:
```
1>F:\svn\llvm\unittests\Support\ErrorTest.cpp(878): error : call to deleted function 'createFileError'
1>F:\svn\llvm\include\llvm/Support/Error.h(1217):  note: candidate function has been explicitly deleted
1>F:\svn\llvm\include\llvm/Support/Error.h(1213):  note: candidate function [with Err = llvm::ErrorSuccess]
```

However, `createFileError("file.bin", MyErrorSuccess())` is fine.

The constructor to `Expected` has the same issue. It would fail with `ErrorSuccess`, but not with `MyErrorSuccess`.

We could make `ErrorSucess` `final`? Is it really desirable to have several ways of succeeding?



Repository:
  rL LLVM

https://reviews.llvm.org/D51490