[PATCH] D53945: [TextAPI] TBD Reader/Writer

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

In https://reviews.llvm.org/D53945#1289270, @amontanez wrote:

> <bikeshed>
>  I noticed the library is named TextAPI, is there a specific reason for this? My patch names the library TAPI, but if there's a reason not to I can change that.
>  </bikeshed>
>
> I'd like to start a conversation on where we'd like the ELF and MachO code paths to diverge. It looks like a thin abstraction of InterfaceFile.h that forks into MachO and ELF specific implementations would be reasonable. TextAPIReaderWriter seems generic enough that it should support an ELF implementation without any modification. In the context of this patch, those places look like reasonable points where implementation specifics might diverge. However, depending on what else would be upstreamed following this patch, I'm not sure if that's the best approach.
>
> An alternative option might be to make this patch provide a single .tbd implementation as independently as possible, and then in separate patches move in the auxiliary (Architecture, Version, Registry, etc.) components later. That would make it significantly easier to provide specific feedback and discussion on whether code is MachO-specific or if some of it could be shared with ELF to reduce code duplication.
>
> The original TAPI is rather large when compared against other llvm libraries, so I'd like to be careful when adding the ELF backend such that it doesn't inflate needlessly. I could adapt the ELF portions to take advantage of existing interfaces after they land, but without more granular review there might be some parts that are specialized and integrated so much that it wouldn't be realistic to do so without completely refactoring existing code.


The interface file has *a lot* of MachO specific things in it. It also isn't clear that even Architecture may not even be easily dedupable IMO. The registry interface seems more on the level of split I was expecting. It demands a raw_ostream for the output which isn't ideal IMO because that's not really how you'd ever want to output an ELF file. I focused on that in my review because of that.


https://reviews.llvm.org/D53945