[PATCH] D34448: bug33527 - Linker::LinkOnlyNeeded should import AppendingLinkage globals

[Benoit Belley via Phabricator via llvm-commits]

belleyb marked 7 inline comments as done.
belleyb added inline comments.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:35
 #include "llvm/LTO/LTO.h"
-#include "llvm/Linker/Linker.h"
 #include "llvm/MC/SubtargetFeature.h"
----------------
tejohnson wrote:
> belleyb wrote:
> > tejohnson wrote:
> > > belleyb wrote:
> > > > Linker isn't used by ThinLTO. Include can be removed.
> > > Can you submit this one as a separate NFC change (no patch review needed)? Thanks.
> > I'm not sure how to proceed as I don't have commit access myself. :-(
> Were planning to get commit access for the patch, or if you want I can commit this part for you now?
I've sent a separate patch for the removal of the include. Would you mind submitting for me ? 

http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170619/463344.html


================
Comment at: unittests/Linker/LinkModulesTest.cpp:364
 
+TEST_F(LinkModuleTest, ImportIntrinsicGlobalVariables_ctors1) {
+  SMDiagnostic Err;
----------------
tejohnson wrote:
> belleyb wrote:
> > tejohnson wrote:
> > > belleyb wrote:
> > > > Starting from here are added unit tests demonstrating the actual issue and proving the suggested fix is correct.
> > > > 
> > > > I was wondering if it would have been more appropriate to write those using llvm-link and FileCheck instead. Let me know if you would prefer that alternate approach instead.
> > > The unit test is quite long - I typically add tests using FileCheck for this type of issue, if you can demonstrate the issue.
> > Sure, I'll try...
> Looking at this again, I see it is not one long unit test but multiple tests. However, I still have a preference for a FileCheck lit test, it is just easier to read and see what is being tested. It looks like it should be reproducible using the inputs you have below and llvm-link, piping the output through FileCheck.
Agreed. Will do this early next week...


https://reviews.llvm.org/D34448