[nfc]Make InstrProfSymtab non-copyable and non-movable #86882
Conversation
- The direct use case (in pr 66825) is to add llvm::IntervalMap and allocator to InstrProfSymtab as class members. The allocator class doesn't have a move-assignment operator; and it's going to take much effort to implement move-assignment operator for the allocator class such that its enclosing class is movable. - There is only one use of compiler-generated move-assignment operator in the repo, which is in CoverageMappingReader.cpp. Luckily it's possible to use std::unique_ptr<InstrProfSymtab> instead in its surrounding code, so did the change.
| // Not copyable or movable. | ||
| // Consider std::unique_ptr for move. | ||
| // InstrProfSymtab has a few containers as class members, so consider | ||
| // std::shared_ptr for read-only copy. |
There was a problem hiding this comment.
std::shared_ptr has (some) overhead and since we aren't using it in this change, lets drop this comment.
There was a problem hiding this comment.
sounds good! I also hesitated about whether to mention shared_ptr and added it mainly for completeness.
| "Caller must provide ProfileNames"); | ||
| std::unique_ptr<BinaryCoverageReader> Reader(new BinaryCoverageReader( | ||
| std::move(ProfileNames), std::move(FuncRecords))); | ||
| InstrProfSymtab &ProfileNamesRef = *Reader->ProfileNames; |
There was a problem hiding this comment.
Can this be const too?
nit: Also prefer just ProfileNames, IMO the Ref suffix is unnecessary.
There was a problem hiding this comment.
Can this be const too?
It seems readCoverageMappingData and its callees only callInstrProfSymtab::getFuncName, which could be made const. A couple of places need to be updated for the const reference in this line to compile. I'll just keep it non const as it currently is..
nit: Also prefer just ProfileNames, IMO the Ref suffix is unnecessary.
Sure! I renamed input parameter to ProfileNamesPtr and rename the reference to ProfileNames.
InstrProfSymtabas owned members. The allocator class doesn't have a move-assignment operator; and it's going to take much effort to implement move-assignment operator for the allocator class such that the enclosing class is movable.