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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: lld/COFF/Driver.cpp:43-44
 using llvm::sys::Process;
-using llvm::sys::fs::file_magic;
-using llvm::sys::fs::identify_magic;
 
----------------
chandlerc wrote:
> If these aren't used, delete the include as well?
The include is still needed.  This file already has `using namespace llvm::object`, and since the two names were moved into the `object` namespace, no other changes are needed to make this work.


================
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
----------------
chandlerc wrote:
> 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.
It was actually in `llvm::sys::fs` before, and I moved it to `object` because it made more sense.  But I can also just put it in `llvm` as you suggest.  Also not keen on making a new namespace just for two or three classes.


================
Comment at: llvm/unittests/ObjectLayout/CMakeLists.txt:2
+set(LLVM_LINK_COMPONENTS
+  ObjectLayout
+  )
----------------
chandlerc wrote:
> Doesn't this depend on Support? (But maybe that works w/o listing it here)
Works fine without it, because the library's `LLVMBuild.txt` references `Support`, so this is automatically picked up.


https://reviews.llvm.org/D33843