[patch] Create a MCSymbolELF

[Duncan P. N. Exon Smith]

> On 2015 Jun 1, at 12:49, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> This create a MCSymbolELF class and moves SymbolSize since only ELF
> needs a size expression.
> 
> This reduces the size of MCSymbol from 56 to 48 bytes.
> 
> Cheers,
> Rafael

This is a great idea.  A few questions/comments below.

> diff --git a/lib/MC/MCContext.cpp b/lib/MC/MCContext.cpp
> index 5756da3..819b468 100644
> --- a/lib/MC/MCContext.cpp
> +++ b/lib/MC/MCContext.cpp
> @@ -20,7 +20,7 @@
>  #include "llvm/MC/MCSectionELF.h"
>  #include "llvm/MC/MCSectionMachO.h"
>  #include "llvm/MC/MCStreamer.h"
> -#include "llvm/MC/MCSymbol.h"
> +#include "llvm/MC/MCSymbolELF.h"
>  #include "llvm/Support/ELF.h"
>  #include "llvm/Support/ErrorHandling.h"
>  #include "llvm/Support/FileSystem.h"
> @@ -119,8 +119,8 @@ MCSymbol *MCContext::getOrCreateSymbol(const Twine &Name) {
>    return Sym;
>  }
>  
> -MCSymbol *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
> -  MCSymbol *&Sym = SectionSymbols[&Section];
> +MCSymbolELF *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
> +  MCSymbolELF *&Sym = SectionSymbols[&Section];
>    if (Sym)
>      return Sym;
>  
> @@ -128,12 +128,12 @@ MCSymbol *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
>  
>    MCSymbol *&OldSym = Symbols[Name];
>    if (OldSym && OldSym->isUndefined()) {
> -    Sym = OldSym;
> -    return OldSym;
> +    Sym = cast<MCSymbolELF>(OldSym);
> +    return Sym;
>    }
>  
>    auto NameIter = UsedNames.insert(std::make_pair(Name, true)).first;
> -  Sym = new (*this) MCSymbol(&*NameIter, /*isTemporary*/ false);
> +  Sym = new (*this) MCSymbolELF(&*NameIter, /*isTemporary*/ false);

It'd be nice to `assert(isELF())` at the top of this function, if that's
not already there.

>  
>    if (!OldSym)
>      OldSym = Sym;
> @@ -163,8 +163,14 @@ MCSymbol *MCContext::CreateSymbol(StringRef Name, bool AlwaysAddSuffix) {
>    if (AllowTemporaryLabels)
>      IsTemporary = Name.startswith(MAI->getPrivateGlobalPrefix());
>  
> -  if (IsTemporary && AlwaysAddSuffix && !UseNamesOnTempLabels)
> -    return new (*this) MCSymbol(nullptr, true);
> +  bool IsELF = MOFI && MOFI->getObjectFileType() == MCObjectFileInfo::IsELF;

Should this be in a function?  When is `MOFI` not available?

> +
> +  if (IsTemporary && AlwaysAddSuffix && !UseNamesOnTempLabels) {
> +    if (IsELF)
> +      return new (*this) MCSymbolELF(nullptr, true);
> +    else
> +      return new (*this) MCSymbol(false, nullptr, true);

Can you add

    /* IsELF */ false

?

[On second thought, just see below...]

> +  }
>  
>    SmallString<128> NewName = Name;
>    bool AddSuffix = AlwaysAddSuffix;
> @@ -178,7 +184,11 @@ MCSymbol *MCContext::CreateSymbol(StringRef Name, bool AlwaysAddSuffix) {
>      if (NameEntry.second) {
>        // Ok, we found a name. Have the MCSymbol object itself refer to the copy
>        // of the string that is embedded in the UsedNames entry.
> -      MCSymbol *Result = new (*this) MCSymbol(&*NameEntry.first, IsTemporary);
> +      MCSymbol *Result;
> +      if (IsELF)
> +        Result = new (*this) MCSymbolELF(&*NameEntry.first, IsTemporary);
> +      else
> +        Result = new (*this) MCSymbol(false, &*NameEntry.first, IsTemporary);

Actually, this is somewhat awkward code duplication.  It might be better
to add a private function like this:

    MCSymbol *MCContext::createSymbolImpl(
        const StringMapEntry<...> *Name, bool IsTemporary) {
      if (IsELF)
        return new (*this) MCSymbolELF(Name, IsTemporary);
      return new (*this) MCSymbol(/* IsELF */ false, Name, IsTemporary);
    }

>        return Result;
>      }
>      assert(IsTemporary && "Cannot rename non-temporary symbols");