[PATCH] D120486: [objcopy] Refactor CommonConfig to add posibility to specify added/updated sections as MemoryBuffer.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:199
+  StringRef SectionName;
+  std::shared_ptr<MemoryBuffer> SectionData;
+};
----------------
avl wrote:
> 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?
`shared_ptr` is almost always a design smell in my experience. The majority of the CopyConfig isn't shared (it's copied), so why should one part of it be? Furthermore, if somebody wanted to update the SectionData for one particular config in their library, but use the original for all the others, they'd end up changing it for all of them unintentionally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120486/new/

https://reviews.llvm.org/D120486