[PATCH] D35386: Speed up gdb index creation

[Rafael Avila de Espindola via llvm-commits]

Rui Ueyama via Phabricator <reviews at reviews.llvm.org> writes:

> ruiu added inline comments.
>
>
> ================
> Comment at: ELF/GdbIndex.cpp:56
> +      continue;
> +    if (auto *M = StringSwitch<LLDDWARFSection *>(Sec->Name)
> +                      .Case(".debug_info", &InfoSection)
> ----------------
> auto -> LLDDWARFSection

Done.

> ================
> Comment at: ELF/GdbIndex.cpp:60
> +                      .Case(".debug_line", &LineSection)
> +                      .Default(nullptr)) {
> +      M->Data = toStringRef(Sec->Data);
> ----------------
> Is this safe?

Yes, it is inside an if checking that M is not null.

> ================
> Comment at: ELF/GdbIndex.cpp:65
> +    }
> +    if (auto *M = StringSwitch<StringRef *>(Sec->Name)
> +                      .Case(".debug_abbrev", &AbbrevSection)
> ----------------
> auto -> StringRef

Done.

> ================
> Comment at: ELF/GdbIndex.cpp:69
> +                      .Case(".debug_gnu_pubtypes", &GnuPubTypesSection)
> +                      .Default(nullptr))
> +      *M = toStringRef(Sec->Data);
> ----------------
> Is returning a nullptr safe?

Yes, same as above.

> I think writing without `StringSwitch` is more straightforward:
>
>   if (Sec->Name == ".debug_abbrev")
>     AbbrevSection = toStringRef...;
>   else if (Sec->Name == ".debug_gnu_pubnames")
>     GnuPubNameSection = toStringRef...;
>   else if ...

Done.

> ================
> Comment at: ELF/GdbIndex.cpp:74
> +
> +template <class ELFT>
> +template <class RelTy>
> ----------------
> I'd write a function comment.

Done.

> ================
> Comment at: ELF/GdbIndex.h:22-26
> +struct LLDDWARFSection final : public llvm::DWARFSection {
> +  InputSectionBase *Sec = nullptr;
> +};
> +
> +template <class ELFT> class LLDDwarfObj final : public llvm::DWARFObj {
> ----------------
> Are these classes designed to be inherited? I prefer delegation over inheritance and am wondering if this is a good approach.

The class is introduced in D35373 and is what llvm's dwarf parser
delegates to.

> ================
> Comment at: ELF/GdbIndex.h:41
> +public:
> +  LLDDwarfObj(elf::ObjectFile<ELFT> *Obj);
> +  const llvm::DWARFSection &getInfoSection() const override {
> ----------------
> explicit?

Done.

I will upload another patch.