[Error] Fix template deduction in createFileError

[aganea]

Summary

This properly fixes the build break here.

/home/buildslave/slave_as-bldslv8/lld-perf-testsuite/llvm/include/llvm/Support/Error.h:1219:11: error: default arguments cannot be added to a function template that has already been declared
        * = nullptr) {
          ^ ~~~~~~~
/home/buildslave/slave_as-bldslv8/lld-perf-testsuite/llvm/include/llvm/Support/Error.h:1175:16: note: previous template declaration is here
  friend Error createFileError(
               ^
1 error generated.

As an example, I've included a compile-time error in ErrorTest.cpp (which I would not commit), although I don't know how I should test this properly?

On a broader view, do you think shielding createFileError() against such usage is useful? It is a corner case, and it is already covered at runtime.

[aganea]

unittests/Support/ErrorTest.cpp

877 ↗

(On Diff #163333)

This is only provided as an example, not to be commited.

MSVC 2017 15.8 fails with:

1>f:\svn\llvm\unittests\support\errortest.cpp(877): error C2672: 'createFileError': no matching overloaded function found
1>f:\svn\llvm\unittests\support\errortest.cpp(877): error C2783: 'llvm::Error llvm::createFileError(std::string,Err)': could not deduce template argument for '<unnamed-symbol>'
1>f:\svn\llvm\include\llvm\support\error.h(1175): note: see declaration of 'llvm::createFileError'

Clang (aug. snapshot) fails with:

1>F:\svn\llvm\unittests\Support\ErrorTest.cpp(877): error : no matching function for call to 'createFileError'
1>F:\svn\llvm\include\llvm/Support/Error.h(1211):  note: candidate template ignored: requirement '!std::is_base_of<ErrorSuccess, ErrorSuccess>::value' was not satisfied [with Err = llvm::ErrorSuccess]

[aganea]

Ping. Please let me know if there’re any comments on this. I just wanted to make sure I’m not overdesigning. TIA!

[zturner]

include/llvm/Support/Error.h

1210–1216 ↗

(On Diff #163333)

What if you just write:

Error createFileError(std::string F, ErrorSuccess) = delete;

Does that also solve the problem?

[ilya-biryukov]

LGTM from my side, SFINAE check looks correct.
Not familiar with the code, though, so we should probably wait for an approval from @zturner too.

include/llvm/Support/Error.h

1210–1216 ↗

(On Diff #163333)

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.

[aganea]

include/llvm/Support/Error.h

1210–1216 ↗

(On Diff #163333)

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?

[ilya-biryukov]

include/llvm/Support/Error.h

1210–1216 ↗

(On Diff #163333)

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)

[ilya-biryukov]

include/llvm/Support/Error.h

1210–1216 ↗

(On Diff #163333)

Correction: making ErrorSuccess final ...

[aganea]

Updated diff. I made ErrorSuccess a final class. That makes things simpler and solves the issue for Expected as well.

Please let me know if that sounds better.

[zturner]

That works, I agree it doesn’t make sense to have descendants of
ErrorSuccess.

Lgtm

[ilya-biryukov]

LG

llvm/trunk/include/llvm/Support/Error.h

#endif
   }
 
   ErrorInfoBase *Payload = nullptr;
 };
 
 /// Subclass of Error for the sole purpose of identifying the success path in
 /// the type system. This allows to catch invalid conversion to Expected<T> at
 /// compile time.
-class ErrorSuccess : public Error {};
+class ErrorSuccess final : public Error {};
 
 inline ErrorSuccess Error::success() { return ErrorSuccess(); }
 
 /// Make a Error instance representing failure using the given error info
 /// type.
 template <typename ErrT, typename... ArgTs> Error make_error(ArgTs &&... Args) {
   return Error(llvm::make_unique<ErrT>(std::forward<ArgTs>(Args)...));
 }

 Error createStringError(std::error_code EC, char const *Msg);
 
 /// This class wraps a filename and another Error.
 ///
 /// In some cases, an error needs to live along a 'source' name, in order to
 /// show more detailed information to the user.
 class FileError final : public ErrorInfo<FileError> {
 
-  template <class Err>
-  friend Error createFileError(std::string, Err);
+  friend Error createFileError(std::string, Error);
 
 public:
   void log(raw_ostream &OS) const override {
     assert(Err && !FileName.empty() && "Trying to log after takeError().");
     OS << "'" << FileName << "': ";
     Err->log(OS);
   }
 
private:
   }
 
   std::string FileName;
   std::unique_ptr<ErrorInfoBase> Err;
 };
 
 /// Concatenate a source file path and/or name with an Error. The resulting
 /// Error is unchecked.
-template <class Err>
-inline Error createFileError(std::string F, Err E) {
+inline Error createFileError(std::string F, Error E) {
   return FileError::build(F, std::move(E));
 }
 
+Error createFileError(std::string F, ErrorSuccess) = delete;
+
 /// Helper for check-and-exit error handling.
 ///
 /// For tool use only. NOT FOR USE IN LIBRARY CODE.
 ///
 class ExitOnError {
 public:
   /// Create an error on exit helper.
   ExitOnError(std::string Banner = "", int DefaultErrorExitCode = 1)

llvm/trunk/unittests/Support/ErrorTest.cpp

 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
     EXPECT_DEATH(
       {
         Error S = Error::success();
         consumeError(createFileError("file.bin", std::move(S)));
       },
       "");
 #endif
+  // Not allowed, would fail at compile-time
+  //consumeError(createFileError("file.bin", ErrorSuccess()));
+
   Error E1 = make_error<CustomError>(1);
   Error FE1 = createFileError("file.bin", std::move(E1));
   EXPECT_EQ(toString(std::move(FE1)).compare("'file.bin': CustomError {1}"), 0);
 
   Error E2 = make_error<CustomError>(2);
   Error FE2 = createFileError("file.bin", std::move(E2));
   handleAllErrors(std::move(FE2), [](const FileError &F) {
     EXPECT_EQ(F.message().compare("'file.bin': CustomError {2}"), 0);