[PATCH] COFFDumper: Dump data directory entries.

Michael Spencer bigcheesegs at gmail.com
Tue Jul 16 15:28:57 PDT 2013


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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130716/1eed5a30/attachment.html>


More information about the llvm-commits mailing list