[llvm-commits] [llvm] r151785 - in /llvm/trunk: include/llvm/Object/ lib/Object/ test/Object/ test/Object/Inputs/ tools/ tools/llvm-readobj/

[David Meyer]

Chandler,

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

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:

A) The type-safe ELF structure definitions
B) Pure ELF object manipulation library (depends on A)
C) ObjectFile interface layer (depends on B)

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.

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.

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

Elf_Dyn is a structure defined in the ELF specification. (Dynamic Table
Entry in .dynamic)

> Is this really the prevailing style of this file? it doesn't at all match
the style of LLVM generally.

Yes.

> Same comment about this comment. Please go through and fix the commenting
throughout this file.

Elf_Rel is also a struct defined in the ELF specs (Elf Relocation)

> WTF is 'isRela'?

Elf_Rela is another ELF structure (ELF Relocation with Addend).

> Please use CamelCase rather than under_scores.
> Please follow LLVM conventions for naming iterators: I and E. Here, and
throughout this file.

These needs to be changed throughout the file.

> Uh, pso-stub.c??? Please fix this comment.

Oops.

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

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.

> And remember that it doesn't need to be a C-style comment any now.
> LLVM orders these after LLVM headers, and uses the C++ naming (cstdio,
cstdlib, etc).
OK.

I will send a patch to fix these things shortly.

- pdox
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120302/72672bdd/attachment.html>