[llvm] r259578 - Correct size calculations for ELF files

Rafael EspĂ­ndola via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 2 14:30:07 PST 2016


This should probably have been reviewed before commit

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

> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-size/basic.test?rev=259578&r1=259577&r2=259578&view=diff
> ==============================================================================
> --- 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.


> +/// @brief Remove unneeded ELF sections from calculation

We don't use @brief. It can be implicit.

> +static bool ConsiderForSize(ObjectFile *Obj, SectionRef Section) {

Function names should start with a lower case.

> +  if (Obj->isELF()) {
> +    switch (static_cast<ELFSectionRef>(Section).getType()) {
> +    case ELF::SHT_NULL:
> +    case ELF::SHT_SYMTAB:
> +    case ELF::SHT_STRTAB:
> +    case ELF::SHT_REL:
> +    case ELF::SHT_RELA:
> +      return false;
> +    }
> +  }
> +  return true;

It seems better to just move the isELF check out of the function and
pass just the section to it.


Given the number of issues for such a small file I would recommend
reverting it recommitting the fixed version.

Cheers,
Rafael


More information about the llvm-commits mailing list