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

[David Blaikie via llvm-commits]

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

> 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?
>

Yeah, seems legit - & quick/easy enough to write (not a lot of engineering
for a corner case), etc.


>
> - 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/685c4729/attachment.html>