[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;
+};
----------------
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`.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:132-133
+      yaml2ObjectFile(Storage, YamlCreationString, ErrHandler);
+  EXPECT_TRUE(Obj);
+  EXPECT_TRUE(IsValidFormat(*Obj));
+
----------------
These two should be ASSERT, since the test will crash if the object isn't valid.


================
Comment at: llvm/unittests/ObjCopy/ObjCopyTest.cpp:162-163
+  ASSERT_THAT_EXPECTED(Result, Succeeded());
+  EXPECT_TRUE(IsValidFormat(**Result));
+  EXPECT_TRUE((*Result)->isObject());
+
----------------
Ditto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120486