[llvm] r264479 - [Support] Switch to RAII helper for error-as-out-parameter idiom.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 25 22:13:37 PDT 2016
Ah, I see what I was missing - boolean testing a failed Error doesn't raise
the checked flag. Carry on.
On Fri, Mar 25, 2016 at 8:27 PM, Lang Hames <lhames at gmail.com> wrote:
> 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/2dfa501f/attachment.html>
More information about the llvm-commits
mailing list