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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/ObjCopy/CommonConfig.h:199
+  StringRef SectionName;
+  std::shared_ptr<MemoryBuffer> SectionData;
+};
----------------
jhenderson wrote:
> 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.
>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? 

There are other parts of CopyConfig which are shared:

```
class NameOrPattern {
  StringRef Name;
  // Regex is shared between multiple CommonConfig instances.
  std::shared_ptr<Regex> R;
  std::shared_ptr<GlobPattern> G;
```

The reason to not copy part of CommonConfig related to Add/Update sections might be intention to avoid copying big piece of data. All other parts of CommonConfig do not require a big storage.

>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.

My understanding is that shared part of data is not supposed to be changed. It Is MemoryBuffer which keeps "const char*". If there would be changed particular SectionData member in the particular config - then it would be perfectly fine and it would not affect others.

If someone would cast away const-ness of MemoryBuffer then it seems to be his responsibility to understand the details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120486