Add support for flattening archives while creating static libraries. As
per cctools' libtool's behavior, llvm-libtool-darwin does not flatten
archives recursively. Depends on D83002.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/tools/llvm-libtool-darwin/invalid-archive.test | ||
|---|---|---|
| 4 | Instead of using a separate Inputs file, you might want to consider using echo to create the input file on the fly. That has the advantage of keeping the test self-contained - you don't have to go looking elsewhere to understand what exactly is going on. | |
| 9 | I take it this behaviour is taken from the original libtool? | |
| 12 | Are you deliberately trying to add the archive symbol index here? That is done implicitly by llvm-ar anyway, so cr is sufficient. | |
| 17 | This needs fixing (you are deleting the archive from the previous case, not the two archives from this case). | |
| 19–20 | Ditto. | |
| 27–31 | Consider using a different archive file name here, so that you don't overwrite the archive from the earlier case. This can be useful in some debugging situations. | |
| llvm/test/tools/llvm-libtool-darwin/valid-archive.test | ||
| 1 | I'd rename this test archive-flattening.test. You should also fold in the invalid cases into it, as there's no need to keep them separate. | |
| 7 | What's the reasoning for deleting %t.lib explicitly here? | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
| 98 | Don't use auto here. It's not obvious what the type is. | |
Comment Actions
LGTM, with the test case reordering. If you want to wait for D83834 too, then that's fine by me.
| llvm/test/tools/llvm-libtool-darwin/archive-flattening.test | ||
|---|---|---|
| 6 ↗ | (On Diff #278193) | I'd reverse the order - put the positive cases before the negative ones. |
| 7–8 ↗ | (On Diff #278193) | It's not committed at the time of writing, but D83834 adds a utility to split an input file into smaller pieces. It might be an even nicer way than using echo to emit the archive. |
| llvm/test/tools/llvm-libtool-darwin/archive-flattening.test | ||
|---|---|---|
| 7–8 ↗ | (On Diff #278193) | Ok, I can wait for it (while I also need to wait for few others to review this diff) Thanks! |
Comment Actions
LGTM
| llvm/test/tools/llvm-libtool-darwin/archive-flattening.test | ||
|---|---|---|
| 27 ↗ | (On Diff #278469) | The archive format is more properly referred to as "Darwin" (at least that's the terminology LLVM uses); the object file format is Mach-O. |
| llvm/test/tools/llvm-libtool-darwin/archive-flattening.test | ||
|---|---|---|
| 27 ↗ | (On Diff #278469) | ok, got it! Thanks |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 97–102 | Thinking through this a little more, Err is passed to the iterator to indicate iteration issues, so you should be checking it inside the loop in each iteration (probably before you call addChildMember). You still need to do an explicit check of Err afterward, so that in the case where the loop didn't iterate at all, the Error is still considered checked (it's guaranteed to be a success in that case, but you still need to do an explicit boolean check to mark it as checked). Perhaps the following would suffice for that? I'm not sure what pattern other parts of the codebase use for this. static_cast<bool>(Err); | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 97–102 | Ah, never mind. The iteration will stop in case of an error, so having the error check after the loop is perfectly fine. (See llvm/include/llvm/ADT/fallible_iterator.) | |
| llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
|---|---|---|
| 97–102 | I had basically this same thought originally too, but this is the pattern for looping through archive members elsewhere, so I realised it was safe. | |