[llvm-commits] Trivial warning

[David Blaikie]

On Fri, Nov 16, 2012 at 11:13 AM, Bill Schmidt
<wschmidt at linux.vnet.ibm.com> wrote:
> On Fri, 2012-11-16 at 12:32 -0600, Hal Finkel wrote:
>> ----- Original Message -----
>> > From: "Roman Divacky" <rdivacky at freebsd.org>
>> > To: "Hal Finkel" <hfinkel at anl.gov>
>> > Cc: llvm-commits at cs.uiuc.edu, "Joe Abbey" <jabbey at arxan.com>
>> > Sent: Friday, November 16, 2012 12:13:53 PM
>> > Subject: Re: [llvm-commits] Trivial warning
>> >
>> > On Fri, Nov 16, 2012 at 11:38:07AM -0600, Hal Finkel wrote:
>> > > ----- Original Message -----
>> > > > From: "Benjamin Kramer" <benny.kra at gmail.com>
>> > > > To: "Joe Abbey" <jabbey at arxan.com>
>> > > > Cc: "Hal Finkel" <hfinkel at anl.gov>, llvm-commits at cs.uiuc.edu
>> > > > Sent: Friday, November 16, 2012 10:00:47 AM
>> > > > Subject: Re: [llvm-commits] Trivial warning
>> > > >
>> > > >
>> > > > On 16.11.2012, at 13:11, Joe Abbey <jabbey at arxan.com> wrote:
>> > > >
>> > > > > On Nov 16, 2012, at 12:38 AM, Joe Abbey <jabbey at arxan.com>
>> > > > > wrote:
>> > > > >
>> > > > >> Hal (et al),
>> > > > >>
>> > > > >> One of my build bots reported a warning:
>> > > > >> PPCRegisterInfo.cpp:501:51: warning: cast from type ???const
>> > > > >> llvm::MachineFunction*??? to type ???llvm::MachineFunction*???
>> > > > >> casts
>> > > > >> away qualifiers [-Wcast-qual]
>> > > > >> Index: lib/Target/PowerPC/PPCRegisterInfo.cpp
>> > > > >> ===================================================================
>> > > > >> --- lib/Target/PowerPC/PPCRegisterInfo.cpp   (revision 168110)
>> > > > >> +++ lib/Target/PowerPC/PPCRegisterInfo.cpp   (working copy)
>> > > > >> @@ -498,7 +498,8 @@
>> > > > >>      } else if (CRSpillFrameIdx) {
>> > > > >>        FrameIdx = CRSpillFrameIdx;
>> > > > >>      } else {
>> > > > >> -      MachineFrameInfo *MFI = ((MachineFunction
>> > > > >> &)MF).getFrameInfo();
>> > > > >> +      MachineFrameInfo *MFI =
>> > > > >> +        (const_cast<MachineFunction &>MF).getFrameInfo();
>> > > > >>        FrameIdx = MFI->CreateFixedObject((uint64_t)4,
>> > > > >>        (int64_t)-4,
>> > > > >>        true);
>> > > > >>        CRSpillFrameIdx = FrameIdx;
>> > > > >>      }
>> > > > >>
>> > > > >> Ok to commit?
>> > > > >
>> > > > > Past Joe,
>> > > > >
>> > > > > Make sure your current build is setup to do PowerPC builds.
>> > > > >
>> > > > > Also, swapping a warning for an error is a horrible trade???
>> > > > > let's
>> > > > > try that patch again:
>> > > > >
>> > > > > Index: lib/Target/PowerPC/PPCRegisterInfo.cpp
>> > > > > ===================================================================
>> > > > > --- lib/Target/PowerPC/PPCRegisterInfo.cpp    (revision 168110)
>> > > > > +++ lib/Target/PowerPC/PPCRegisterInfo.cpp    (working copy)
>> > > > > @@ -498,7 +498,8 @@
>> > > > >      } else if (CRSpillFrameIdx) {
>> > > > >        FrameIdx = CRSpillFrameIdx;
>> > > > >      } else {
>> > > > > -      MachineFrameInfo *MFI = ((MachineFunction
>> > > > > &)MF).getFrameInfo();
>> > > > > +      MachineFrameInfo *MFI =
>> > > > > +        (const_cast<MachineFunction &>(MF)).getFrameInfo();
>> > > > >        FrameIdx = MFI->CreateFixedObject((uint64_t)4,
>> > > > >        (int64_t)-4,
>> > > > >        true);
>> > > > >        CRSpillFrameIdx = FrameIdx;
>> > > > >      }
>> > > >
>> > > > While this would be the right way to silence the warning, the
>> > > > whole
>> > > > thing looks like a horrible hack to me. Shouldn't the FrameIdx
>> > > > generated in a place where MF isn't const?
>> > >
>> > > Roman, you had looked at these const issues, right? Do you have an
>> > > opinion on this?
>> >
>> > I actively avoided any const_cast<>s in my const rage :)
>> >
>> > This code was contributed by wschmidt@ and iirc it needs to be
>> > like this because PPC is special (addressing against fp).
>>
>> Bill, opinion?
>>
>
> It looks to me like this has been changed a couple of times now:
>
> Original code:
>       MachineFrameInfo *MFI = ((MachineFunction &)MF).getFrameInfo();
>
> Fixed in 165941 by Micah Villnow:
>
> 165941   mvillmow       MachineFrameInfo *MFI =
> (const_cast<MachineFunction &>(MF)).getFrameInfo();
>
> Reverted by Chandler in 167222:
>
> 167222  chandlerc       MachineFrameInfo *MFI = ((MachineFunction
> &)MF).getFrameInfo();

It looks like this particular change was reverted without prejudice.
The change was part of a much larger & more problematic change that
was reverted on those merits - this was just collateral damage.

>
> And now it's under discussion again...
>
> I don't know of a better way to do this, so if anyone feels it's too
> much of a bloody hack, I welcome suggestions.  Seems to me like the
> const_cast is the best alternative, but it's probably worth asking
> Chandler why he reverted Micah's change.
>
> My two cents,
> Bill
>
>
>
>> >
>> > I think Bill didnt see an easy way to fix this. I think const_cast<>
>> > in here is better than C style cast.
>>
>> Agreed.
>>
>>  -Hal
>>
>> >
>> > Roman
>> >
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits