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

[Lang Hames via llvm-commits]

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.

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