This is an archive of the discontinued LLVM Phabricator instance.

[LTO] NFC: remove unique_ptr from some LTO::RegularLTOState members
AbandonedPublic

Authored by vitalybuka on Dec 14 2017, 11:44 PM.

Details

Reviewers
pcc
Summary

LTO::RegularLTOState::CombinedModule and LTO::RegularLTOState::Mover do not have
to be pointers.

LTO::RegularLTOState::HasCombinedModule is needed to have this patch as NFC.
It should be removed by D41267.

Event Timeline

vitalybuka created this revision.Dec 14 2017, 11:44 PM

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

llvm/include/llvm/Transforms/Utils/SplitModule.h
39

You're changing the semantic here by not transferring ownership of the module.
This is not a const reference, which indicate side-effects on the module, but you don't document what are these.

pcc added inline comments.Dec 15 2017, 11:02 AM
llvm/include/llvm/Transforms/Utils/SplitModule.h
37

Sorry, I forgot about the fact that this function takes a unique_ptr. In that case it would probably be fine to leave the CombinedModule field as a unique_ptr but just move its initialization to the constructor.

vitalybuka abandoned this revision.Dec 15 2017, 11:15 AM

This patch doesn't seem motivated, the only effect I can see is that a ThinLTO link that don't include any LTO module would create a module while it was lazily created only if needed before.

Yes, this was motivated only by comment on D41267.