[llvm] r259578 - Correct size calculations for ELF files

[Rafael Espíndola via llvm-commits]

On 3 February 2016 at 12:22, Hemant Kulkarni <khemant at codeaurora.org> wrote:
>
>>> Added:
>> >    llvm/trunk/test/tools/llvm-size/Inputs/
>>>     llvm/trunk/test/tools/llvm-size/Inputs/1.o
>
>
>>Why do you need to check in a object file? For size you should be able to use a .s and llvm-mc. It should also have a better name.
>
> Will change the name and add an assembly file.

And delete the object file.

>>> --- llvm/trunk/test/tools/llvm-size/basic.test (original)
>>> +++ llvm/trunk/test/tools/llvm-size/basic.test Tue Feb  2 15:41:49
>>> +++ 2016
>>> @@ -1,2 +1,28 @@
>>>  RUN: llvm-size %t.blah 2>&1 | FileCheck --check-prefix=ENOENT %s
>
>>This is a completely different test, please use a new file.
> If this is named correctly then I believe this is right place to add. It is only testing basic operation of llvm-size nothing fancy.

1.o is not a reasonable name. In this case it should just be replaced
with assembly, but if in the future you need to add an object file,
please follow the pattern used in other tests: A test named "foo.test"
uses a file named "Inputs/Foo.o".

>
>>> +/// @brief Remove unneeded ELF sections from calculation
>
>>We don't use @brief. It can be implicit.
> I am being consistent other functions in this file have this same style.

Please update the file or change this. Please don't propagate old style.

>>> +static bool ConsiderForSize(ObjectFile *Obj, SectionRef Section) {
>>Function names should start with a lower case.
> I see other function names in this file starting with upper case. Again I am only being consistent here.

Please change them. Please don't propagate old style or we will never
finish moving out of it.

>>It seems better to just move the isELF check out of the function and pass just the section to it.
> Can you explain why?

The entire body of the function is nested in the if.

>>Given the number of issues for such a small file I would recommend reverting it recommitting the fixed version.
> This patch is not breaking anything but only correcting the tool. Instead of adding 2 commits (1 revert another reformatted/refactored commit with NFC)
> I am of opinion any changes we agree should simply go in a patch that correts any issues.

It makes it much harder to see that the final change implements
everything. Please revert and commit a new patch with the fix.

Cheers,
Rafael