Chandler,<div><br></div><div><div>> In the future, I might break this large of a patch into a few separate commits... I have several comments inline. Sorry if I missed a subsequent patch from you addressing these things, but I'm responding to what I see here.</div>
<div>> There are a lot of style issues with this patch. I don't know how many are due to inherent style issues permeating the Object library, but I'd really like to at least stop making things worse and preferably start improving the consistency and style of this library w.r.t. other libraries in LLVM. See comments inline.</div>

</div><div><br></div><div>There are a lot of serious stylistic issues with lib/Object. It needs some major refactoring. I figure Object/ELF.h should be broken up into at least three pieces:</div><div><br></div>
<div>A) The type-safe ELF structure definitions</div>
<div>B) Pure ELF object manipulation library (depends on A)</div><div>C) ObjectFile interface layer (depends on B)</div><div><br></div><div>I'm not sure who owns this code and wants to do the re-factoring. I've been passing my changes by Michael Spencer. I think he did much of the initial implementation.</div>

<div><br></div><div>We need some high-level discussion about how to make this interface and implementation cleaner. But in these changes, I've been matching the style of the existing code, despite it being quite unusual.</div>

<div><br></div><div><div>> This is a terrible comment. This is Elf_Dyn_Base. What is it? Please write complete English sentences, and make the comments doxygen comments. This isn't just an implementation detail either, this is in a major public header of the Object library.</div>

</div><div><br></div><div>Elf_Dyn is a structure defined in the ELF specification. (Dynamic Table Entry in .dynamic)</div><div><br></div><div><div>> Is this really the prevailing style of this file? it doesn't at all match the style of LLVM generally.</div>

</div><div><br></div><div>Yes.</div><div><br></div><div><div>> Same comment about this comment. Please go through and fix the commenting throughout this file.</div></div><div><br></div><div>Elf_Rel is also a struct defined in the ELF specs (Elf Relocation)</div>

<div><br></div><div><div>> WTF is 'isRela'?</div></div><div><br></div><div>Elf_Rela is another ELF structure (ELF Relocation with Addend).</div><div><br></div><div>> Please use CamelCase rather than under_scores.</div>

<div>> Please follow LLVM conventions for naming iterators: I and E. Here, and throughout this file.</div><div><br></div><div>These needs to be changed throughout the file.</div><div><br></div><div>> Uh, pso-stub.c??? Please fix this comment.</div>

<div><br></div><div>Oops.</div><div><br></div><div>> Also, where did this comment go??? You can't just add a tool with zero documentation. ;] You also need to add this tool to the commandline tools documentation webpages, manual pages, etc. Also, please add something to the release notes about this work.</div>

<div><br></div><div><div>I created readobj in heat-of-the-moment desperation to get some real testing out of the ObjectFile interface. I'm not sure how far we want to go with supporting this as a utility.</div><div><br>

</div></div><div><div>> And remember that it doesn't need to be a C-style comment any now.</div></div><div><div>> LLVM orders these after LLVM headers, and uses the C++ naming (cstdio, cstdlib, etc).</div>
</div><div>OK.</div><div><br>I will send a patch to fix these things shortly.</div><div><br></div><div>- pdox</div>