[PATCH] D126883: [Debuginfo][DWARF][NFC] Refactor DwarfStringPoolEntryRef.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

In D126883#3582133 <https://reviews.llvm.org/D126883#3582133>, @JDevlieghere wrote:

> Almost, what I had in mind was this:
>
>   DwarfStringPoolEntry {
>     ...
>   }
>   
>   DwarfStringPoolEntryRefImpl {
>     // Everything that *doesn't* depend on whether it's backed by a DwarfStringPoolEntry or DwarfStringPoolEntry*. This should be able to cover your entire public interface. 
>   }
>   
>   template<typename T>
>   DwarfStringPoolEntryRef : public DwarfStringPoolEntryRefImpl {
>     // Everything that *does* depend on the type of T. 
>   }
>   
>   // The (Non)OwningDwarfStringPoolEntryRef are now just typedefs.
>   typedef OwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;
>   typedef NonOwningDwarfStringPoolEntryRef = DwarfStringPoolEntryRef<DwarfStringPoolEntry*>;
>   
>   // The accelerator table can remain non-templated by only interacting with the DwarfStringPoolEntryRefImpl:
>   
>   template class AccelTable : public AccelTableBase {
>   public:
>     template <typename EntryT, typename... Types>
>     void addName(DwarfStringPoolEntryRefImpl& Name, Types &&... Args);
>   };

yep, thank you.

It looks like this solution does not cover one original property of DwarfStringPoolEntryRef. 
Current DwarfStringPoolEntryRef assumed to be passed by value. 
That means that no neccessary to have extra storage to keep it:

CodeGen/AccelTable.h

  struct HashData {
    DwarfStringPoolEntryRef Name;
    ...
  } 

CodeGen/DIE.h

  class DIEString {
    DwarfStringPoolEntryRef S;
  }

If we will pass it by reference:

CodeGen/AccelTable.h

  struct HashData {
    DwarfStringPoolEntryRefImpl& Name;
    ...
  } 

CodeGen/DIE.h

  class DIEString {
    DwarfStringPoolEntryRefImpl& S;
  }

Then we need to add some containers and do more copies for OwningDwarfStringPoolEntryRef or NonOwningDwarfStringPoolEntryRef:

  template class AccelTable : public AccelTableBase {
  public:
    template <typename EntryT, typename... Types>
    void addName(DwarfStringPoolEntryRefImpl& Name, Types &&... Args);
  };

The addName should call special copy for the Name(which would copy real object referenced by DwarfStringPoolEntryRefImpl) and store it in an additional container.

  class DIEString {
    DwarfStringPoolEntryRef& S;
  }

The DIEString should be changed somehow like this:

  class DIEString {
    std::unique_ptr<DwarfStringPoolEntryRef> S;
  }

Would it be OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126883