[PATCH] D60127: Declare library dependency introduced by https://reviews.llvm.org/D59482

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D60127#1451390 <https://reviews.llvm.org/D60127#1451390>, @labath wrote:

> In D60127#1451385 <https://reviews.llvm.org/D60127#1451385>, @jhenderson wrote:
>
> > I can't see how D59482 <https://reviews.llvm.org/D59482> introduces a dependency on LLVMObject in the main LLVMObjectYAML library. It does add unit tests that rely on the Object library, and includes the necessary reference. Where is the new dependency that this is needed for?
>
>
> I think serge has linked the wrong patch. The culprit is https://reviews.llvm.org/D59634, which does add the dependency because it uses the Object library to construct the ObjectYAML model. This is a new dependency because for all the other binary formats the code to construct the yaml model lives in tool code (obj2yaml). Here, I chose to do that in the ObjectYAML library because I wanted to make that code accessible to unit tests.


Ah, that makes more sense, thanks.

> @jhenderson do you see any problems with this new dependency (i.e., should I push a patch adding it), or does it need more discussion (i.e., revert the original patch) ? (FWIW, to me it seems natural for ObjectYAML to depend on Object, and I don't think this creates any danger of dependency cycles, as Object should never depend on ObjectYAML.)

My main concern is the number of files needed to build obj2yaml/yaml2obj, which would dramatically increase with that dependency. That being said, most of those files would be needed to be built for the tools that these two executables are used to test, so I don't think it's a genuine issue, and what you say about naturalness does make sense, so please go ahead and fix it.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60127/new/

https://reviews.llvm.org/D60127