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

[Ilya Biryukov via Phabricator via llvm-commits]

ilya-biryukov 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));
 }
----------------
aganea wrote:
> 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?
> 
Making `ErrorSuccess` sounds reasonable.
The sfinae trick also checks the deduced type is a subtype of `Error`, do we want to get rid of this check too? (Would fail in the template body instead, I assume)


Repository:
  rL LLVM

https://reviews.llvm.org/D51490