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

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 07:31:16 PDT 2017


tejohnson 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"
----------------
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.


================
Comment at: unittests/Linker/LinkModulesTest.cpp:364
 
+TEST_F(LinkModuleTest, ImportIntrinsicGlobalVariables_ctors1) {
+  SMDiagnostic Err;
----------------
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.


https://reviews.llvm.org/D34448





More information about the llvm-commits mailing list