[lld] r221974 - Follow-up to r221913. Fix some -Wcast-qual warning reasons.

[David Blaikie]

On Fri, Nov 14, 2014 at 12:03 AM, Simon Atanasyan <simon at atanasyan.com>
wrote:

> On Fri, Nov 14, 2014 at 10:24 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >
> > On Thu, Nov 13, 2014 at 11:15 PM, Simon Atanasyan <simon at atanasyan.com>
> > wrote:
>
> [...]
>
> >>
> ==============================================================================
> >> --- lld/trunk/lib/Core/SymbolTable.cpp (original)
> >> +++ lld/trunk/lib/Core/SymbolTable.cpp Fri Nov 14 01:15:43 2014
> >> @@ -187,9 +187,10 @@ bool SymbolTable::addByName(const Atom &
> >>    case NCR_Second:
> >>      useNew = true;
> >>      break;
> >> -  case NCR_DupDef:
> >> -    switch (mergeSelect(cast<DefinedAtom>(existing)->merge(),
> >> -                        cast<DefinedAtom>(&newAtom)->merge())) {
> >> +  case NCR_DupDef: {
> >> +    const auto *existingDef = cast<DefinedAtom>(existing);
> >> +    const auto *newDef = cast<DefinedAtom>(&newAtom);
> >
> >
> > These casts should already be safe & could remain as they were before, I
> > believe. the llvm::cast machinery preserves const without having to
> specify
> > it explicitly.
>
> You are right, the llvm::cast machinery preserves const. But I prefer
> to use the const qualifier with the auto keyword to emphasize the
> constness of a variable. Besides that, the same pattern is used a few
> lines below in the NCR_DupUndef case.
>

I hope you could do that as cast<const T>(...)->... if you want to be
explicit about the const (if that doesn't work - we'll fix the bug in the
llvm::cast machinery) - but if you prefer the named variables, that's cool
too.


>
> [...]
>
> >>
> ==============================================================================
> >> --- lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h (original)
> >> +++ lld/trunk/lib/ReaderWriter/ELF/Mips/MipsELFFile.h Fri Nov 14
> 01:15:43
> >> 2014
> >> @@ -216,9 +216,9 @@ private:
> >>      case llvm::ELF::R_MIPS_32:
> >>      case llvm::ELF::R_MIPS_GPREL32:
> >>      case llvm::ELF::R_MIPS_PC32:
> >> -      return *(int32_t *)ap;
> >> +      return *(const int32_t *)ap;
> >
> >
> > This and all casts like it are likely UB (if the underlying pointer is
> just
> > pointing to bytes, not an actual int32_t object) and we should be using
> our
> > utility functions (I forget where they are or what they're called) that
> can
> > read from bytes into ints, etc (using memcpy, the blessed way to do this
> in
> > a well defined manner).
>
> Yeah, it is in my TODO list. I plan to clean up that code when start
> to implement big-endian target support.
>
> --
> Simon Atanasyan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141114/d7f4c606/attachment.html>