[PATCH] D113130: [llvm-libtool-darwin] Throw an error if object file names are repeated

[Roger Kim via Phabricator via llvm-commits]

Roger added inline comments.


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:105-108
+  std::vector<NewArchiveMember> Members;
+  // This vector contains the file that each NewArchiveMember from Members came
+  // from. Therefore, it has the same size as Members.
+  std::vector<StringRef> Files;
----------------
thevinster wrote:
> These vectors feel a bit strange to me. It almost feels like NewArchiveMember should either support filename in the struct or a much less invasive change is to have this class inherit NewArchiveMember and support the filename field here. We can always downcast it if methods elsewhere require the NewArchiveMember object. 
The main reason why I went with this approach is because other functions `writeArchive` and `writeArchiveToBuffers` depend on getting a `std::vector<NewArchiveMember>` as an input. If that were not the case, I would have done something like

```
struct NewArchiveMemberWrapper {
  NewArchiveMember Member;
  StringRef File;
};
```

and used `std::vector<NewArchiveMemberWrapper>` instead of `std::vector<NewArchiveMember>`.

> It almost feels like NewArchiveMember should either support filename in the struct or a much less invasive change is to have this class inherit NewArchiveMember and support the filename field here.

I intentionally avoid using inheritance here because, fundamentally, I am not trying to create another type of NewArchiveMember. I am just trying to tie an additional piece of data (a file path) to each NewArchiveMember. Also, if I created a child class (let's call it `NewArchiveMemberChild`) and used `std::vector<NewArchiveMemberChild>`, then I would have to do extra work to generate the `std::vector<NewArchiveMember>` that the `writeArchive` and `writeArchiveToBuffer` functions need. Using composition here is simpler and I believe more appropriate.

I also did not add a new StringRef field to `NewArchiveMember` because the feature I am introducing this field for is specific to `llvm-libtool-darwin` but `NewArchiveMember` is not (it is shared among multiple libraries). Adding a new field to `NewArchiveMember` would be leaking state/complexity relevant only to `llvm-libtool-darwin` out to other libraries.

> These vectors feel a bit strange to me.

I will admit one thing I do not like is that the `Members` vector and the `Files` vector are programmed to always have the same size but this invariant is not implied by their types. If we had one vector of `NewArchiveMemberWrapper`, then this wouldn't be an issue but, again, the existing code does not make this easy. I think it might be do-able if we had the ranges-v3 library or C++20's ranges but we do not :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113130