[llvm] r264479 - [Support] Switch to RAII helper for error-as-out-parameter idiom.
Lang Hames via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 20:27:25 PDT 2016
Hi Dave,
That will assert-fail - there's a unit test for it.
Just so we've got our nomenclature clear:
(1) The checked bit is true if the error has been checked, and false
otherwise. We want it to be false after the constructor exits to force
clients to check the result.
(2) Boolean conversion for Error is: Error -> true, Success -> false.
So the ErrorAsOutParameter destructor is doing the right thing: If the
user's constructor assigned an error at any point the checked flag will
have been cleared, and the ErrorAsOutParameter destructor will not set it
(boolean conversion doesn't set the flag if there's an error). If the
user's constructor did not assign an error (i.e. !Err is true) then the
ErrorAsOutParameter destructor will assign a fresh success value to Err,
which will clear the flag.
Sometimes having a 'checked' flag feels a bit backwards - I wonder if
having an 'unchecked' flag would actually be clearer - that way the flag
value would mirror the boolean conversion operator result (true on error,
false otherwise).
- Lang.
On Fri, Mar 25, 2016 at 5:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Fri, Mar 25, 2016 at 4:54 PM, Lang Hames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: lhames
>> Date: Fri Mar 25 18:54:32 2016
>> New Revision: 264479
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=264479&view=rev
>> Log:
>> [Support] Switch to RAII helper for error-as-out-parameter idiom.
>>
>> As discussed on the llvm-commits thread for r264467.
>>
>> Modified:
>> llvm/trunk/include/llvm/Support/Error.h
>> llvm/trunk/lib/Object/MachOObjectFile.cpp
>> llvm/trunk/unittests/Support/ErrorTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/Support/Error.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Error.h?rev=264479&r1=264478&r2=264479&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Support/Error.h (original)
>> +++ llvm/trunk/include/llvm/Support/Error.h Fri Mar 25 18:54:32 2016
>> @@ -143,13 +143,6 @@ public:
>> /// constructor, but should be preferred for readability where
>> possible.
>> static Error success() { return Error(); }
>>
>> - /// Create a 'pre-checked' success value suitable for use as an
>> out-parameter.
>> - static Error errorForOutParameter() {
>> - Error Err;
>> - (void)!!Err;
>> - return Err;
>> - }
>> -
>> // Errors are not copy-constructable.
>> Error(const Error &Other) = delete;
>>
>> @@ -552,6 +545,36 @@ inline void consumeError(Error Err) {
>> handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
>> }
>>
>> +/// Helper for Errors used as out-parameters.
>> +///
>> +/// This helper is for use with the Error-as-out-parameter idiom, where
>> an error
>> +/// is passed to a function or method by reference, rather than being
>> returned.
>> +/// In such cases it is helpful to set the checked bit on entry to the
>> function
>> +/// so that the error can be written to (unchecked Errors abort on
>> assignment)
>> +/// and clear the checked bit on exit so that clients cannot
>> accidentally forget
>> +/// to check the result. This helper performs these actions
>> automatically using
>> +/// RAII:
>> +///
>> +/// Result foo(Error &Err) {
>> +/// ErrorAsOutParameter ErrAsOutParam(Err); // 'Checked' flag set
>> +/// // <body of foo>
>> +/// // <- 'Checked' flag auto-cleared when ErrAsOutParam is destructed.
>> +/// }
>> +class ErrorAsOutParameter {
>> +public:
>> + ErrorAsOutParameter(Error &Err) : Err(Err) {
>> + // Raise the checked bit if Err is success.
>> + (void)!!Err;
>> + }
>> + ~ErrorAsOutParameter() {
>> + // Clear the checked bit.
>> + if (!Err)
>>
>
> Wouldn't this raise the checked bit if there /was/ an error?
>
> { Error e; ErrorAsOutParameter(e); e = /* some actual error */; }
>
> That should assert-fail, but I suspect it won't?
>
>
>> + Err = Error::success();
>> + }
>> +private:
>> + Error &Err;
>> +};
>> +
>> /// Tagged union holding either a T or a Error.
>> ///
>> /// This class parallels ErrorOr, but replaces error_code with Error.
>> Since
>>
>> Modified: llvm/trunk/lib/Object/MachOObjectFile.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Object/MachOObjectFile.cpp?rev=264479&r1=264478&r2=264479&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Object/MachOObjectFile.cpp (original)
>> +++ llvm/trunk/lib/Object/MachOObjectFile.cpp Fri Mar 25 18:54:32 2016
>> @@ -246,7 +246,7 @@ static Error parseSegmentLoadCommand(
>> Expected<std::unique_ptr<MachOObjectFile>>
>> MachOObjectFile::create(MemoryBufferRef Object, bool IsLittleEndian,
>> bool Is64Bits) {
>> - Error Err = Error::errorForOutParameter();
>> + Error Err;
>> std::unique_ptr<MachOObjectFile> Obj(
>> new MachOObjectFile(std::move(Object), IsLittleEndian,
>> Is64Bits, Err));
>> @@ -262,7 +262,7 @@ MachOObjectFile::MachOObjectFile(MemoryB
>> DataInCodeLoadCmd(nullptr), LinkOptHintsLoadCmd(nullptr),
>> DyldInfoLoadCmd(nullptr), UuidLoadCmd(nullptr),
>> HasPageZeroSegment(false) {
>> -
>> + ErrorAsOutParameter ErrAsOutParam(Err);
>> if (is64Bit())
>> parseHeader(this, Header64, Err);
>> else
>>
>> Modified: llvm/trunk/unittests/Support/ErrorTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264479&r1=264478&r2=264479&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/unittests/Support/ErrorTest.cpp (original)
>> +++ llvm/trunk/unittests/Support/ErrorTest.cpp Fri Mar 25 18:54:32 2016
>> @@ -105,12 +105,32 @@ TEST(Error, UncheckedSuccess) {
>> }
>> #endif
>>
>> -// Test that errors to be used as out parameters are implicitly checked (
>> -// and thus destruct quietly).
>> -TEST(Error, ErrorAsOutParameter) {
>> - Error E = Error::errorForOutParameter();
>> +// ErrorAsOutParameter tester.
>> +void errAsOutParamHelper(Error &Err) {
>> + ErrorAsOutParameter ErrAsOutParam(Err);
>> + // Verify that checked flag is raised - assignment should not crash.
>> + Err = Error::success();
>> + // Raise the checked bit manually - caller should still have to test
>> the
>> + // error.
>> + (void)!!Err;
>> }
>>
>> +// Test that ErrorAsOutParameter sets the checked flag on construction.
>> +TEST(Error, ErrorAsOutParameterChecked) {
>> + Error E;
>> + errAsOutParamHelper(E);
>> + (void)!!E;
>> +}
>> +
>> +// Test that ErrorAsOutParameter clears the checked flag on destruction.
>> +#ifndef NDEBUG
>> +TEST(Error, ErrorAsOutParameterUnchecked) {
>> + EXPECT_DEATH({ Error E; errAsOutParamHelper(E); },
>> + "Program aborted due to an unhandled Error:")
>> + << "ErrorAsOutParameter did not clear the checked flag on
>> destruction.";
>> +}
>> +#endif
>> +
>> // Check that we abort on unhandled failure cases. (Force conversion to
>> bool
>> // to make sure that we don't accidentally treat checked errors as
>> handled).
>> // Test runs in debug mode only.
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160325/db2eb7c2/attachment.html>
More information about the llvm-commits
mailing list