[PATCH] COFFDumper: Dump data directory entries.

[Rui Ueyama]

On Tue, Jul 16, 2013 at 2:19 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> > Size may be from an input file so it can be a very large number. I think
> the extra condition is to check for integer overflow. We shouldn't use
> assert() here because if it fails it means the input file is broken and
> it's not a bug of this code.
>
> We don't have special cases for 64 bit integers overflowing in other
> parts of the codebase. You are right, it cannot be an assert. It would
> be interesting to have some form of fatal error instead, but you don't
> need to do that in this patch.
>
> > ================
> > Comment at: lib/Object/COFFObjectFile.cpp:613
> > @@ +612,3 @@
> > +                                            const data_directory *&Res)
> const {
> > +  // Error if if there's no data directory or the index is out of range.
> > +  if (!DataDirectory || index > PE32Header->NumberOfRvaAndSize)
> > ----------------
> > Rafael Ávila de Espíndola wrote:
> >> Just returing a data_directory* (possibly null) would be a simpler
> interface.
> > All the other get* functions have the same return type. I think it's
> nice to keep it consistent with others.
>
>  A good part of this API has a way more general interface than what is
> needed. Returning an error_code to me at least gives some wrong
> impressions:
>
> * The getter is doing actual work (like parsing), and can really fail.
> * There are multiple possible ways it can fail.
>
> Since it is the constructor that is doing the actual work, just
> returning a pointer is preferable. Getters like these are fairly
> common in LLVM.
>

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