[PATCH] COFFDumper: Dump data directory entries.

Rafael EspĂ­ndola rafael.espindola at gmail.com
Thu Jul 18 13:40:14 PDT 2013


I would still prefer to not push forward with the unnecessary use of
error_code, but Michael's review is certainly sufficient.

On 18 July 2013 16:16, Rui Ueyama <ruiu at google.com> wrote:
> It's not very clear to me if this patch is stamped. Rafael, are you ok with
> this patch?
>
>
> On Tue, Jul 16, 2013 at 3:28 PM, Michael Spencer <bigcheesegs at gmail.com>
> wrote:
>>
>> On Tue, Jul 16, 2013 at 2:52 PM, Rui Ueyama <ruiu at google.com> wrote:
>>>
>>> Actually this code is outside of lld and was written mainly by Michael.
>>> I'd like to get his input about API rework. If he's okay with API rework I'm
>>> fine to work on that. However, what I'm trying to do is to add a relatively
>>> small method to the set of methods which spans multiple files in a
>>> consistent manner, so I'm tempted to say API rework shouldn't block this
>>> change.
>>>
>>>
>>> On Tue, Jul 16, 2013 at 2:41 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>>
>>>>
>>>> On Tue, Jul 16, 2013 at 2:37 PM, Rafael EspĂ­ndola
>>>> <rafael.espindola at gmail.com> wrote:
>>>>>
>>>>> > I agree with you in general but in this case consistency looks more
>>>>> > important than that. Look at the other methods, such as the above
>>>>> > two.
>>>>> > getCOFFHeader() and getPE32Header() will never fail and will always
>>>>> > return
>>>>> > object_error::success. Other methods in the other files in the same
>>>>> > directory are written in the similar manner. If we change the
>>>>> > signature of
>>>>> > this function, it won't fix the API, but it'd make the API cluttered.
>>>>> >
>>>>> > I'd think we should change all the other methods in the directory not
>>>>> > to
>>>>> > return error_code, if we think it's the right thing to do. That
>>>>> > should be
>>>>> > done in another patch.
>>>>>
>>>>> I think we should change it all, yes. If you don't want to have the
>>>>> methods temporarily in a mixed state, then please change the existing
>>>>> ones first.
>>>>
>>>>
>>>> Rafael, I don't think that's necessary. This is all code that Rui has
>>>> written and been maintaining. If he would prefer to do the API rework in a
>>>> follow-up patch that seems totally fine.
>>>
>>>
>>
>> Error handling can be fixed separately. It's a problem in all of
>> libObject.
>>
>> lgtm.
>>
>> - Michael Spencer
>>
>




More information about the llvm-commits mailing list