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

[Lang Hames via llvm-commits]

Actually, maybe RAII is the way to go here:

Instead of:

Foo::Foo(Error &Err) {
  // ...
  if (Cond)
    Err = ...
  // ...
}

Expected<Foo> Foo::create() {
  Error Err = Error::errorForOutParameter();
  Foo F(Err);
  if (Err)
    return std::move(Err);
  return std::move(F);
}

We could have:

Foo::Foo(Error &Err) {
  ErrorForOutParam _(Err); // '_' clears checked flag at entry, unclears it
at exit.
  // ...
  if (Cond)
    Err = ...
  // ...
}

Expected<Foo> Foo::create() {
  Error Err;
  Foo F(Err);
  if (Err)
    return std::move(Err);
  return std::move(F);
}

What do you think?

- Lang.


On Fri, Mar 25, 2016 at 3:14 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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/0c058388/attachment.html>