Page MenuHomePhabricator

[llvm-link][NFC] Minor cleanup
ClosedPublic

Authored by sdmitriev on Dec 8 2020, 11:31 PM.

Details

Summary

llvm::Linker::linkModules() is a static member, so there is no need
to pass reference to llvm::Linker instance to loadArFile() function.

Diff Detail

Event Timeline

sdmitriev requested review of this revision.Dec 8 2020, 11:31 PM
sdmitriev created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 11:31 PM
tra added a comment.Dec 9 2020, 9:29 AM

llvm::Linker::linkModules() is a static member, so there is no need
to pass reference to llvm::Linker instance to loadArFile() function.

I think you are relying on implementation details here. All you know within the llvm-link.cpp is that you're operating on an instance of class Linker. How it's implemented and whether Linker::linkModules happens to be a static member may change at any point in time.
Eliminating one function argument does not buy us anything here, as far as I can see.

Well, when you use a class you have to know the interface it defines, so this is not about the implementation details, but about the interface provided by the class. llvm::Linker::linkModules(), as it is defined today, is a static member of the class, so it does not require an instance of llvm::Linker class to be called. Adding an additional parameter to loadArFile just in case does not buy as anything at all except unneeded redundancy. And, by the way, this function is called as a static member (i.e. Linker::linkModules()) everywhere in LLVM sources except llvm-link.cpp:

llvm/lib/Linker/LinkModules.cpp, line 605
llvm/tools/bugpoint/BugDriver.cpp, line 149
llvm/tools/bugpoint/Miscompilation.cpp, lines 233, 396, ...
llvm/unittests/Linker/LinkModulesTest.cpp, lines 101, 175, 182, ...
tra accepted this revision.Dec 9 2020, 5:57 PM

And, by the way, this function is called as a static member (i.e. Linker::linkModules()) everywhere in LLVM sources except llvm-link.cpp:

Fair enough. Making the calls consistent is beneficial.

This revision is now accepted and ready to land.Dec 9 2020, 5:57 PM
This revision was landed with ongoing or failed builds.Dec 9 2020, 11:18 PM
This revision was automatically updated to reflect the committed changes.