[llvm] r264595 - [llvm-readobj] NFC Replace case by macros for PT_* enums

[ChrisBieneman via llvm-commits]

The patch won't leak because they're all stack allocations, but this patch is kinda gross. Incurring substr on constant strings is kinda nasty.

Since all you're doing with the substr call is removing the PT_, wouldn't it be cleaner to make the macro add PT_ to the enum name, the second parameter specified in the macro could just be a constant string that gets returned.

In general I think this function is a bit odd because the only case that really *needs* a std::string is the error condition, in all other cases it could be const char*. This might be a good place to use one of the libSupport error constructs.

-Chris

> On Mar 29, 2016, at 12:36 PM, Hemant Kulkarni via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> I do not think it is a leak. I am not doing any explicit heap allocation.
> This will dellocate when it goes out of function scope.
> 
> --
> Hemant Kulkarni
> khemant at codeaurora.org
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
> the Linux Foundation
> 
> 
> -----Original Message-----
> From: Alex Rosenberg [mailto:alexr at ohmantics.com] 
> Sent: Tuesday, March 29, 2016 11:56 AM
> To: Hemant Kulkarni <khemant at codeaurora.org>
> Cc: llvm-commits at lists.llvm.org
> Subject: Re: [llvm] r264595 - [llvm-readobj] NFC Replace case by macros for
> PT_* enums
> 
> Doesn't substr return a new string object? Doesn't this then leak memory?
> 
> Alex
> 
> On Mar 28, 2016, at 10:20 AM, Hemant Kulkarni via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> 
>> Author: khemant
>> Date: Mon Mar 28 12:20:23 2016
>> New Revision: 264595
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=264595&view=rev
>> Log:
>> [llvm-readobj] NFC Replace case by macros for PT_* enums
>> 
>> Modified:
>>   llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
>> 
>> Modified: llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-readobj/ELFD
>> umper.cpp?rev=264595&r1=264594&r2=264595&view=diff
>> ======================================================================
>> ========
>> --- llvm/trunk/tools/llvm-readobj/ELFDumper.cpp (original)
>> +++ llvm/trunk/tools/llvm-readobj/ELFDumper.cpp Mon Mar 28 12:20:23 
>> +++ 2016
>> @@ -44,6 +44,10 @@ using namespace ELF; #define ENUM_ENT_1(enum) \
>>  { #enum, #enum, ELF::enum }
>> 
>> +#define LLVM_READOBJ_PHDR_ENUM(ns, enum)
> \
>> +  case ns::enum:
> \
>> +    return std::string(#enum).substr(3);
>> +
>> #define TYPEDEF_ELF_TYPES(ELFT)
> \
>>  typedef ELFFile<ELFT> ELFO;
> \
>>  typedef typename ELFO::Elf_Shdr Elf_Shdr;
> \
>> @@ -1085,30 +1089,18 @@ static const char *getElfSegmentType(uns
>> 
>> static std::string getElfPtType(unsigned Arch, unsigned Type) {
>>  switch (Type) {
>> -  case ELF::PT_NULL:
>> -    return "NULL";
>> -  case ELF::PT_LOAD:
>> -    return "LOAD";
>> -  case ELF::PT_DYNAMIC:
>> -    return "DYNAMIC";
>> -  case ELF::PT_INTERP:
>> -    return "INTERP";
>> -  case ELF::PT_NOTE:
>> -    return "NOTE";
>> -  case ELF::PT_SHLIB:
>> -    return "SHLIB";
>> -  case ELF::PT_PHDR:
>> -    return "PHDR";
>> -  case ELF::PT_TLS:
>> -    return "TLS";
>> -  case ELF::PT_GNU_EH_FRAME:
>> -    return "GNU_EH_FRAME";
>> -  case ELF::PT_SUNW_UNWIND:
>> -    return "SUNW_UNWIND";
>> -  case ELF::PT_GNU_STACK:
>> -    return "GNU_STACK";
>> -  case ELF::PT_GNU_RELRO:
>> -    return "GNU_RELRO";
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_NULL)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_LOAD)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_DYNAMIC)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_INTERP)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_NOTE)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_SHLIB)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_PHDR)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_TLS)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_GNU_EH_FRAME)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_SUNW_UNWIND)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_GNU_STACK)
>> +    LLVM_READOBJ_PHDR_ENUM(ELF, PT_GNU_RELRO)
>>  default:
>>    // All machine specific PT_* types
>>    switch (Arch) {
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits