[llvm] r264467 - [Support] Add Error::errorForOutParameter helper.

[David Blaikie via llvm-commits]

On Fri, Mar 25, 2016 at 3:11 PM, Lang Hames <lhames at gmail.com> wrote:

> Hi Dave,
>
> The constructor should assign to Error no matter what (I may have failed
> to assign success in the MachOObjectFile constructor - I'll check), and
> consequently the caller will have to check no matter what. The issue is
> with the assignment operator. It's an error to overwrite an unchecked
> value, so the following will fail:
>
> Error Err = Error::success();
> Err = Error::success(); // <- Failure to check the first success value.
>
> Using
>
> Error = Err::errorForOutParameter();
> Error = Error::success();
>
> Instead is safe.
>

Right... hmm, tricky. :/ yeah, not sure of the best way to account for all
of that & make it least error prone in these use cases.


>
> - Lang.
>
> On Fri, Mar 25, 2016 at 3:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Fri, Mar 25, 2016 at 2:56 PM, Lang Hames via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: lhames
>>> Date: Fri Mar 25 16:56:35 2016
>>> New Revision: 264467
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=264467&view=rev
>>> Log:
>>> [Support] Add Error::errorForOutParameter helper.
>>>
>>> This helper method creates a pre-checked Error suitable for use as an out
>>> parameter in a constructor. This avoids the need to have the constructor
>>> check a known-good error before assigning to it.
>>>
>>
>> Hmm, thinking about it - how does this work to ensure safety?
>>
>> If the callee only assigns to the parameter on failure cases, won't the
>> code fail to assert on successful cases?
>>
>> (ie: shouldn't it actually be unchecked by default as it was before - the
>> caller should check it on return regardless of what the function did)
>>
>>
>>>
>>>
>>> Modified:
>>>     llvm/trunk/include/llvm/Support/Error.h
>>>     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=264467&r1=264466&r2=264467&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/Support/Error.h (original)
>>> +++ llvm/trunk/include/llvm/Support/Error.h Fri Mar 25 16:56:35 2016
>>> @@ -143,6 +143,13 @@ 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;
>>>
>>>
>>> Modified: llvm/trunk/unittests/Support/ErrorTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/ErrorTest.cpp?rev=264467&r1=264466&r2=264467&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/unittests/Support/ErrorTest.cpp (original)
>>> +++ llvm/trunk/unittests/Support/ErrorTest.cpp Fri Mar 25 16:56:35 2016
>>> @@ -105,6 +105,12 @@ 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();
>>> +}
>>> +
>>>  // 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/976132e2/attachment.html>