[PATCH] D120486: [objcopy] Refactor CommonConfig to add posibility to specify added/updated sections as MemoryBuffer.
Alexey Lapshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 28 04:08:03 PST 2022
avl added inline comments.
================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:199
+ StringRef SectionName;
+ std::shared_ptr<MemoryBuffer> SectionData;
+};
----------------
jhenderson wrote:
> jhenderson wrote:
> > avl wrote:
> > > jhenderson wrote:
> > > > avl wrote:
> > > > > jhenderson wrote:
> > > > > > Why `shared_ptr`?
> > > > > There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic. unique_ptr should be copied with "move" semantic. if unique_ptr would be used here then we need to write strange copy operator:
> > > > >
> > > > >
> > > > > ```
> > > > > NewSectionInfo& operator=(const NewSectionInfo& Other) {
> > > > > SectionData = std::move(const_cast<NewSectionInfo*>(&Other)->SectionData);
> > > > > }
> > > > > ```
> > > > >
> > > > >
> > > > > There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic
> > > >
> > > > Do they need to copy though? It seems like a reference should be enough.
> > > > > There are many cases where CommonConfig and ConfigManager are copied using "copy" semantic
> > > >
> > > > Do they need to copy though? It seems like a reference should be enough.
> > >
> > > parseStripOptions fills DC.CopyConfigs by copying of initial Config :
> > >
> > >
> > > ```
> > > for (StringRef Filename : Positional) {
> > > ...
> > > Config.InputFilename = Filename;
> > > Config.OutputFilename = Filename;
> > > DC.CopyConfigs.push_back(ConfigMgr);
> > >
> > > ```
> > >
> > > "move" semantic could not be used here.
> > Okay. I'm wondering whether we could change the structure of how that works to allow a core config that has everything except the filenames in, and then that can be taken by reference, or some similar design.
> >
> > However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.
> > However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.
>
> Are you planning on looking at this? I still think you shouldn't have a `shared_ptr`.
>> However, actually, I think it would just be simpler to implement a copy constructor that makes a copy of the memory buffer - you already have code that does this in this patch as it is.
>Are you planning on looking at this? I still think you shouldn't have a shared_ptr.
Oh, sorry, I misunderstood the comment.
With shared_ptr we do not need to copy of the memory buffer. For unique_ptr we need to copy of the memory buffer. But then we might have some performance degradation. Would it be OK?
Why using shared_ptr is a problem?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120486/new/
https://reviews.llvm.org/D120486
More information about the llvm-commits
mailing list