[PATCH] D64281: [Object] Create MutableObject class for doing Object Mutations [Part 1]

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 22 09:32:33 PDT 2019


jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:34
+
+  MutableELFSection(uintptr_t ToCopy, const MutableELFObject<ELFT> *ObjFile)
+      : MutableELFSection(toDataRef(ToCopy), ObjFile) {}
----------------
abrachet wrote:
> jhenderson wrote:
> > I think a more normal-looking signature would simply be `MutableELFSection(uint8_t *Data, const MutableELFObject<ELF> *ObjFile)`
> I'm not sure I understand why it should be a `uint8_t *`, `ELFObjectFile::getSection` takes a `DataRef`
Right, sorry. I incorrectly assumed that DataRefImpl would store a pointer, since it essentially is a pointer to some data.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:52
+  Expected<StringRef> getSectionName(DataRefImpl Sec) const override {
+    auto Mapping = Sections[Sec.p];
+    if (Mapping.New) {
----------------
abrachet wrote:
> jhenderson wrote:
> > Probably shouldn't use auto here.
> The typename is really long would you be open to changing it back to auto?
No. LLVM follows the rule of "only use auto when it is clear from the context what the type is". See the Coding Standards.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:51
+    if (Mapping.New) {
+      auto *NewSec = Sections.getNew(Mapping.Ptr);
+      return NewSec->Name;
----------------
This is another use of auto that should probably change. You might want to typedef the MappingType from earlier.


================
Comment at: llvm/include/llvm/Object/MutableELFObject.h:72
+  MutableELFSection<ELFT> *getMutableSection(section_iterator Sec) {
+    auto Index = Sec->getRawDataRefImpl().p;
+    return Sections.makeMutable(Index, this);
----------------
More too much auto.


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

https://reviews.llvm.org/D64281





More information about the llvm-commits mailing list