[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
david-salinas wrote: @kikairoya Thank you so much for patching this! Greatly appreciated. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
kikairoya wrote: I think `ExtractBundleEntryOptsTableGen` is needed here. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: Thanks, yes that would do it. @kikairoya or @Andarwinux, feel free to commit the fix, to avoid needing a revert. I don't have easy access to an LLVM development environment currently. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: @david-salinas, it looks like the build bot failure is related to this PR. I'm not sure why it's only impacting one specific build bot though, given the nature of the failure. Either way, this should be investigated and reverted if the fix is going to take some time to come. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
https://github.com/kikairoya edited https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
Andarwinux wrote: This broke build with LLVM_TOOL_LLVM_DRIVER_BUILD=ON. @jh7370 Can you revert this PR now? https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
https://github.com/jh7370 edited https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
https://github.com/jh7370 approved this pull request. LGTM, thanks and sorry for the slow speed at reviewing this! https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: Yes, that's the default, but I think it's still possible to build for a 32-bit architecture (and it's quite possible there are architectures out there which are technically 64-bit but with 32-bit `size_t` type after all). https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: > Running the test on Windows (successfully) doesn't really give us any more > test coverage I don't think it's really any different to most tests in LLVM: we generally try to make the test function on Windows and Linux hosts (and Mach-O and so on), even if the functionality is Linux specific, unless it would require things that simply aren't available on a given OS. I think the logic is that a Windows developer should be able to modify the code in this area without needing a Linux setup and be able to rely on the tests covering them from accidentally breaking things. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
@@ -1717,15 +1717,15 @@ objcopy::parseStripOptions(ArrayRef
RawArgsArr,
return std::move(DC);
}
-Error llvm::objcopy::runExtractBundleEntry(SmallVector args) {
+Error llvm::objcopy::runExtractBundleEntry(SmallVector args) {
jh7370 wrote:
1) args -> Args
2) Shouldn't this be being passed by `const &`?
3) I think the norm is to pass the `SmallVectorImpl` type instead of the
`SmallVector` itself, as we don't care about the size parameter (or lack
thereof) in this context.
https://github.com/llvm/llvm-project/pull/169386
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: This still hasn't been addressed. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Add llvm-extract-bundle-entry to extend llvm-objcopy (PR #169386)
jh7370 wrote: This doesn't seem to have been resolved? You're still mentioning Driver Config, when it doesn't need to be mentioned in the comment as it's not in the interface anywhere. https://github.com/llvm/llvm-project/pull/169386 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
