[PATCH] D33843: Create a new library called ObjectLayout, and move headers from Support to there.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Since the name discussion seems winding down, review on the code changes themselves...

Generally, there are a lot of unrelated #include order changes. Would it be ok if you (or I) do a widespread clang-format run over the #include lines to get them sorted so that your patch doesn't change that much? I'd like to avoid reverts or build bot trouble due to include order at the same time as trying to land the new library. Remaining (minor) comments inline.

In https://reviews.llvm.org/D33843#773101, @zturner wrote:

> Anybody else have thoughts on the name?


Peter Collingborne mentioned on IRC he liked `BinaryFormat` as well. I suggest going with that for now. Ship it! (Worst case, we can rename it later if needed...)



================
Comment at: lld/COFF/Driver.cpp:43-44
 using llvm::sys::Process;
-using llvm::sys::fs::file_magic;
-using llvm::sys::fs::identify_magic;
 
----------------
If these aren't used, delete the include as well?


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2645-2646
 /// A metadata node with a DWARF macro info (i.e., a constant named
-/// \c DW_MACINFO_*, defined in llvm/Support/Dwarf.h).  Called \a DIMacroNode
+/// \c DW_MACINFO_*, defined in llvm/ObjectLayout/Dwarf.h).  Called \a
+/// DIMacroNode
 /// because it's potentially used for non-DWARF output.
----------------
weird comment formatting.


================
Comment at: llvm/include/llvm/ObjectLayout/Magic.h:17
+namespace llvm {
+namespace object {
+/// file_magic - An "enum class" enumeration of file types based on magic (the
----------------
This probably shouldn't be in the `object` namespace any more. And `format` seems confusing. I guess `binary_format` would be unambiguous, but yuck.

I would probably just put this in the `llvm` namespace, and come back through here with improved names for all the things within it.


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:2598
+// public defintions.  So the are here not in include/llvm/ObjectLayout/MachO.h
+// .
 
----------------
Weird comment reformat...


================
Comment at: llvm/unittests/ObjectLayout/CMakeLists.txt:2
+set(LLVM_LINK_COMPONENTS
+  ObjectLayout
+  )
----------------
Doesn't this depend on Support? (But maybe that works w/o listing it here)


https://reviews.llvm.org/D33843