[llvm] r259578 - Correct size calculations for ELF files

[Hemant Kulkarni via llvm-commits]

>> 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.

>> --- 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.


>> +/// @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.

>> +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.



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

>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.


--
Hemant Kulkarni
khemant at codeaurora.org
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by the Linux Foundation