[PATCH] D61636: [llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names"

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:405
                                    uint8_t Visibility, uint16_t Shndx,
                                    uint64_t Size) {
   Symbol Sym;
----------------
jhenderson wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Maybe a better fix would be to rename this parameter to SymbolSize, to avoid any future confusion?
> > But it will be not consistent with `Type`, `Value` and others. Doesn't seem we want to
> > rename all of those to `SymbolType`, `SymbolValue` etc?
> If another person comes along and writes something to do with the section size in this function, then I could easily see them make the same mistake. Also, at least on Visual Studio with high warning levels, the compiler emits warnings for shadowed variables (and this would have highlighted this issue here), so we should allow people to build with high warning levels by fixing this. I don't mind the name being slightly inconsistent, if it makes the code less fragile. I also am not strongly against `SymbolType`, SymbolValue etc, if you feel consistency is important.
`LLVM_ENABLE_WARNINGS` defaults to on => `/W4` for MSVC. This shadowing emits a warning:

arning C4458: declaration of 'size' hides class member


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

https://reviews.llvm.org/D61636