<div dir="ltr"><div class="gmail_extra">On Tue, Jul 16, 2013 at 2:19 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>

</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">

> 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.<br>


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

<div><br></div><div>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.</div></div></div></div>