[PATCH] D16820: Implement common symbol size calculation in llvm-size

[Hemant Kulkarni via llvm-commits]

I will make the blakent style change after this patch. For now I am making the changes you mentioned.

Thanks.
--
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: Rafael Espíndola [mailto:rafael.espindola at gmail.com] 
Sent: Tuesday, March 08, 2016 8:07 AM
To: reviews+D16820+public+ddfe081b9477909e at reviews.llvm.org
Cc: Hemant Kulkarni; llvm-commits; Davide Italiano
Subject: Re: [PATCH] D16820: Implement common symbol size calculation in llvm-size

On 1 March 2016 at 17:38, khemant at codeaurora.org <khemant at codeaurora.org> wrote:
> khemant added inline comments.
>
> ================
> Comment at: test/tools/llvm-size/X86/test-common.s:17
> @@ +16,3 @@
> +       movl    $1, x
> +       movl    $1, y
> +       movl    $1, z
> ----------------
> rafael wrote:
>> Do you actually need all this? To test just the common size I don't see you need any asm instruction.
> I needed some section with non zero size for case where no common is specified.

So just a nop will do, right?

> Does that matter?

Yes. Please try to keep test as small as possible to make it obvious what is being tested.

>
> ================
> Comment at: tools/llvm-size/llvm-size.cpp:370 @@ +369,3 @@
> +    if (ELFCommons) {
> +      uint64_t common_size = getCommonSize(Obj);
> +      total += common_size;
> ----------------
> rafael wrote:
>> CommonSize
> There are variables with all lower case. Do you want me to converst all locals to upper before merging this patch with upper case variables pertaining to this change?

Not in the same patch, but a previous patch that just fixes the existing style would be awesome.

Cheers,
Rafael