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

[Jim Grosbach via llvm-commits]

Ping? These review comments really need to be addressed.


> On Mar 29, 2016, at 11:37 PM, ChrisBieneman via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 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
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits