[PATCH] D74205: [llvm-dwarfdump] Add the --show-sections-sizes option

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added inline comments.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/macho_archive_section_sizes.test:1
+RUN: llvm-dwarfdump --show-section-sizes %S/../../dsymutil/Inputs/libfat-test.a \
+RUN:   | FileCheck %s --match-full-lines --strict-whitespace
----------------
jhenderson wrote:
> djtodoro wrote:
> > jhenderson wrote:
> > > You should avoid using a precanned binary if possible, especially one from a completely different tool's testing. yaml2obj has Mach-O support, so I'd recommend using that (although I don't know how to create fat archives).
> > > 
> > > The new file should have section names in the ELF style too, to show that they are not included.
> > >You should avoid using a precanned binary if possible, especially one from a completely different tool's testing. yaml2obj has Mach-O support, so I'd recommend using that (although I don't know how to create fat archives).
> > There are cases, even in llvm-dwarfdump, using the files from Input/, but if you think so, I can delete it.
> > 
> > >The new file should have section names in the ELF style too, to show that they are not included.
> > I've added a new test case for this.
> Using assembly etc from the Inputs folder local to the current tool is fine, but where possible, we try to avoid pre-canned binaries these days, as they are opaque and hard to maintain. Referencing anotehr tool's Inputs folder is particularly bad as it creates an unexpected dependency between the two sets of tests.
OK, I'll remove the test.


================
Comment at: llvm/tools/llvm-dwarfdump/SectionSizes.cpp:80-81
+
+namespace llvm {
+
+void calculateSectionSizes(const ObjectFile &Obj, SectionSizes &Sizes,
----------------
jhenderson wrote:
> djtodoro wrote:
> > jhenderson wrote:
> > > You still don't need the `namespace llvm` since you have a `using namespace llvm` at the top of this file.
> > This is needed to avoid an ambiguous call to the `calculateSectionSizes()`.
> What ambiguous call? There is only one function called `calculateSectionSizes()` defined here...
Without even `llvm::` here I get:

  SectionSizes.cpp:109:3: error: call to 'calculateSectionSizes' is ambiguous

maybe I am oversighting something very obvious here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74205/new/

https://reviews.llvm.org/D74205