In file 'clang/lib/Basic/Targets.cpp' the function 'AllocateTarget' had a raw pointer as a return type, which have been wrapped in the 'std::unique_ptr' in all usages.
This commit changes the signature of the function to return an instance of 'std::unique_ptr' directly.
Details
Diff Detail
Event Timeline
That's my first commit into LLVM project. (Almost just for test of the contribution procedure.)
Maybe I did something wrong, but the build fails on example tests.
I wonder, whether this review request will be actually reviewed despite the failed condition? And maybe someone can provide some (simple) tutorials/guides how to contribute here in more correct way.
That's my first commit into LLVM project. (Almost just for test of the contribution procedure.)
Welcome!
Maybe I did something wrong, but the build fails on example tests.
Do the tests fail locally? I will look at the pre-commit logs as well. It may be failures that were fixed already and this change needs to be rebased to include that.
I wonder, whether this review request will be actually reviewed despite the failed condition? And maybe someone can provide some (simple) tutorials/guides how to contribute here in more correct way.
Yes, absolutely review isn't just about criticising code it's about solving problems.
The intent of this change is great (down with raw pointers) and at first glance I don't think it should cause any failures.
Yes, it fails locally even for clean repository. I mean, just cloned main branch and tested immediately without any changes.
I have found another review, which has the same tests failed. https://reviews.llvm.org/D148546
Moreover, you (@DavidSpickett) have already commented it.
Seems like it is more general problem, but I can't figure out how to fix it.
I'd suggest rewording the title a bit: [clang] Return std::unique_ptr<TargetInfo> from AllocateTarget
Titles are always weird but at least including the function name means someone staring at a build failure can skim for likely commits. It's not always possible though.
Same results for me without this change:
Failed Tests (6): LLVM :: Examples/OrcV2Examples/lljit-with-remote-debugging.test LLVM :: Examples/OrcV2Examples/lljit-with-thinlto-summaries.test LLVM :: Examples/OrcV2Examples/orcv2-cbindings-add-object-file.test LLVM :: Examples/OrcV2Examples/orcv2-cbindings-basic-usage.test LLVM :: Examples/OrcV2Examples/orcv2-cbindings-lazy.test LLVM :: Examples/OrcV2Examples/orcv2-cbindings-removable-code.test
The examples are not built by default you have to give LLVM_BUILD_EXAMPLES=ON to CMake.
It's not your problem to fix those, this patch doesn't cause them so it's fine to go in as is.
Do you have commit access? https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
If you want I can land this for you, I need a name and email address to use for the commit.
Do you have commit access?
No, I don't. And, according to the Policy (as I understood), someone with the commit access have to commit my changes prior I can request the access too.
If you want I can land this for you, I need a name and email address to use for the commit.
My nickname for GitHub: Stoorx
My email: me[at]stoorx.one
Or should I provide a real name?
Or should I provide a real name?
Username is fine as long as you're fine being credited as such.
Username is fine as long as you're fine being credited as such.
Absolutely.
What should I do further?
You may get some emails about build failures. Try to judge if they are relevant to you, ask here if you are not sure.
I doubt it will do much given that it's just refactoring and both uses seem to be GPU offload targets.
What should I do further?
And contribute more if you like :)
Thanks for the contribution!