17 Apr
2012
17 Apr
'12
4:22 p.m.
Darrell, all
I'm making an attempt at fixing k3b for gcc47. If anyone else has already done
this please stop me. It is part of bug 958.
--
David C. Rankin, J.D.,P.E.
18 Apr
18 Apr
11:25 a.m.
New subject: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)
On 04/17/2012 06:22 PM, David C. Rankin wrote:
Darrell, all
I'm making an attempt at fixing k3b for gcc47. If anyone else has already done
this please stop me. It is part of bug 958.
This is another redeclaration issue, but it is not the same type of simple
iterator issue I've seen earlier. The build of k3b fails on gcc47 with:
k3baudioeditorwidget.cpp: In member function 'virtual void
K3bAudioEditorWidget::mousePressEvent(TQMouseEvent*)':
k3baudioeditorwidget.cpp:674:12: error: redeclaration of
'K3bAudioEditorWidget::Range* r'
k3baudioeditorwidget.cpp:668:14: error: 'K3bAudioEditorWidget::Range* r'
previously declared here
It is complaining about 'r' being declared twice below. This looks more like a
reassignment than a redeclaration to me. How do we properly fix it?. If all we
care about here is 'r' having function scope, can't I just get rid of the
'Range* ' typecast following the else { ? Or do I have to change 'r' to
'not_r'
following the else?
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if( Range* r = findRangeEdge( e->pos(), &end ) ) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange( r );
}
else {
Range* r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
As long as it is just used in mousePressEvent, can't I just do:
else {
r = findRange( e->pos() );
Obviously, the compiler is taking the 'if( Range* r' as the declaration, so it
should still know what 'r' is after the else.... What say the experts...
--
David C. Rankin, J.D.,P.E.
11:48 a.m.
New subject: SOLVED [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
On 04/18/2012 01:25 PM, David C. Rankin wrote:
It is complaining about 'r' being declared
twice below. This looks more like a
reassignment than a redeclaration to me. How do we properly fix it?. If all we
care about here is 'r' having function scope, can't I just get rid of the
'Range* ' typecast following the else { ? Or do I have to change 'r' to
'not_r'
following the else?
Getting rid of the second 'Range* ' was the trick. Verify and then push patch :)
--
David C. Rankin, J.D.,P.E.
12:08 p.m.
New subject: [trinity-devel] SOLVED [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
It is
complaining about 'r' being declared twice below. This looks more like a
reassignment than a redeclaration to me. How do we properly fix it?. If all we
care about here is 'r' having function scope, can't I
just get rid of
the 'Range* ' typecast following the else { ? Or do I have
to change 'r' to 'not_r' following the else?
Getting rid of the second 'Range* ' was the trick. Verify and then push patch :)
12:12 p.m.
New subject: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)
On Wed, 18 Apr 2012 13:25:43 -0500
"David C. Rankin" <drankinatty(a)suddenlinkmail.com> wrote:
On 04/17/2012 06:22 PM, David C. Rankin wrote:
If I'm understanding the code correctly, I would probably move the declaration
of r outside the condition for the if, that is:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) {
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
Range* r = findRangeEdge(e->pos(), &end);
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
m_draggedMarker = findMarker( e->pos() );
}
setSelectedRange( r );
TQFrame::mousePressEvent(e);
}
To my eye, that's clearer than the original. I would never declare a variable
inside a condition for a branching construct, even if it's legal to do so (the
technique has valid applications in loop constructs, but here it just serves to
muddy the scoping). </opinionated>
Alternatively, basic scoping rules suggest:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if( Range* r = findRangeEdge( e->pos(), &end ) ) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange( r );
}
else {
Range* r_two = findRange( e->pos() );
d->movedRange = r_two;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r_two );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
Darrell, all
I'm making an attempt at fixing k3b for gcc47. If anyone else has already done
this please stop me. It is part of bug 958.
This is another redeclaration issue, but it is not the same type of simple
iterator issue I've seen earlier. The build of k3b fails on gcc47 with:
k3baudioeditorwidget.cpp: In member function 'virtual void
K3bAudioEditorWidget::mousePressEvent(TQMouseEvent*)':
k3baudioeditorwidget.cpp:674:12: error: redeclaration of
'K3bAudioEditorWidget::Range* r'
k3baudioeditorwidget.cpp:668:14: error: 'K3bAudioEditorWidget::Range* r'
previously declared here
It is complaining about 'r' being declared twice below. This looks more like a
reassignment than a redeclaration to me. How do we properly fix it?. If all we
care about here is 'r' having function scope, can't I just get rid of the
'Range* ' typecast following the else { ? Or do I have to change 'r' to
'not_r'
following the else?
12:33 p.m.
New subject: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)
On 04/18/2012 02:12 PM, E. Liddell wrote:
On Wed, 18 Apr 2012 13:25:43 -0500
"David C. Rankin" <drankinatty(a)suddenlinkmail.com> wrote:
Darrell, E (don't know first name)
I agree with all your comment and both proposals above, but like Darrell, I
don't have a c++ background to know the 'best' way to do this. I certainly
agree
what declaration within the conditional looks bad, but I was not going to take
the liberty and re-write it given the conservative approach I take when
monkeying with the code (though I like the declaration outside the if statement
much better as suggested). I know they are not big changes, but I don't know
enough to know if they were declared in the if for some other reason.
Of the two, I like the first suggestion the best, just from a style/clarity
standpoint:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) {
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
Range* r = findRangeEdge(e->pos(), &end);
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
m_draggedMarker = findMarker( e->pos() );
}
setSelectedRange( r );
TQFrame::mousePressEvent(e);
}
I'll change it any way you guys want it changed. The way it is currently
patched works fine (k3b runs fine as well), but it isn't the prettiest. Let me
know which option you like the best and I'll redo the patch. I'm just glad k3b
is building and working on gcc47 without and -fpermissive kludge :)
--
David C. Rankin, J.D.,P.E.
On 04/17/2012 06:22 PM, David C. Rankin wrote:
If I'm understanding the code correctly, I would probably move the declaration
of r outside the condition for the if, that is:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e ) {
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
Range* r = findRangeEdge(e->pos(), &end);
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
m_draggedMarker = findMarker( e->pos() );
}
setSelectedRange( r );
TQFrame::mousePressEvent(e);
}
To my eye, that's clearer than the original. I would never declare a variable
inside a condition for a branching construct, even if it's legal to do so (the
technique has valid applications in loop constructs, but here it just serves to
muddy the scoping). </opinionated>
Alternatively, basic scoping rules suggest:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if( Range* r = findRangeEdge( e->pos(), &end ) ) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange( r );
}
else {
Range* r_two = findRange( e->pos() );
d->movedRange = r_two;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r_two );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
Darrell, all
I'm making an attempt at fixing k3b for gcc47. If anyone else has already done
this please stop me. It is part of bug 958.
This is another redeclaration issue, but it is not the same type of simple
iterator issue I've seen earlier. The build of k3b fails on gcc47 with:
k3baudioeditorwidget.cpp: In member function 'virtual void
K3bAudioEditorWidget::mousePressEvent(TQMouseEvent*)':
k3baudioeditorwidget.cpp:674:12: error: redeclaration of
'K3bAudioEditorWidget::Range* r'
k3baudioeditorwidget.cpp:668:14: error: 'K3bAudioEditorWidget::Range* r'
previously declared here
It is complaining about 'r' being declared twice below. This looks more like a
reassignment than a redeclaration to me. How do we properly fix it?. If all we
care about here is 'r' having function scope, can't I just get rid of the
'Range* ' typecast following the else { ? Or do I have to change 'r' to
'not_r'
following the else?
1:37 p.m.
New subject: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)
On Wed, 18 Apr 2012 14:33:36 -0500
"David C. Rankin" <drankinatty(a)suddenlinkmail.com> wrote:
Darrell, E (don't know first name)
E is fine. I'm not all that fond of my first name, and prefer not to use it.
I agree with all your comment and both proposals
above, but like Darrell, I
don't have a c++ background to know the 'best' way to do this. I certainly
agree
what declaration within the conditional looks bad, but I was not going to take
the liberty and re-write it given the conservative approach I take when
monkeying with the code (though I like the declaration outside the if statement
much better as suggested). I know they are not big changes, but I don't know
enough to know if they were declared in the if for some other reason.
I don't exactly have a C++ background either (I coded trivial stuff in it for about
a year and a half as a CS grad student, going on fifteen years ago), but my suspicion
in this case is that the original programmer was trying to save himself keystrokes
and/or had taken up that idiom in order to save memory back in the days when
RAM was measured in kilobytes (declaring r outside the if statement would keep it in
scope just a little bit longer in most languages). Either that, or I've forgotten
some
obscure but vital point about C++ conditionals and scoping.
1:26 p.m.
New subject: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)
On 04/18/2012 02:12 PM, E. Liddell wrote:
void K3bAudioEditorWidget::mousePressEvent(
TQMouseEvent* e ) {
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
Range* r = findRangeEdge(e->pos(),&end);
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
m_draggedMarker = findMarker( e->pos() );
}
setSelectedRange( r );
TQFrame::mousePressEvent(e);
}
What say we do it like this:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
Range* r = findRangeEdge( e->pos(), &end );
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange(r);
}
else {
= findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
Here is why -- looking at the setSelectedRange code it has something to do
with repainting the screen, which brings up the question in my mind: "Does
setSelectedRange( r ); have to come 'before' m_draggedMarker = findMarker(
e->pos() ); -- as it is originally. That's the only reason I could think of
that the original author would have put it both after the 'if' and after the
'else' -- otherwise, I agree with you, just put it at the end.
I'm not sure how to check in the code to see if need to be before the
findMarker( e->pos() ); call. Can somebody who knows this stuff let us know?
Either way, the proposed patch above preserves the order of the calls --
the only remaining question is can we git rid of both setSelectedRange(r);
calls by moving a single call below the conditional... Small potatoes there...
Let me know. Thanks.
--
David C. Rankin, J.D.,P.E.
1:52 p.m.
New subject: 2nd Patch [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
On 04/18/2012 03:36 PM, David C. Rankin wrote:
On 04/18/2012 03:26 PM, David C. Rankin wrote:
Here is what I have come up with from the additional posts here. I've updated
the patch to bug report 958. The link to the new patch is here:
http://bugs.pearsoncomputing.net/attachment.cgi?id=549
The code reads:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
Range* r = findRangeEdge( e->pos(), &end );
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange( r );
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
You guys that do the approval before Darrell submits, take a look and let us
know if it is good. I'm on to the next one..
--
David C. Rankin, J.D.,P.E.
else {
= findRange( e->pos() );
with the 'r' = of course :)
2:37 p.m.
New subject: [trinity-devel] 2nd Patch [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
On Wed, 18 Apr 2012 15:52:26 -0500
"David C. Rankin" <drankinatty(a)suddenlinkmail.com> wrote:
On 04/18/2012 03:36 PM, David C. Rankin wrote:
Are you sure that compiles? I would have assumed end needed to be declared before
using a reference to it as an argument to findRangeEdge(), which was why I put things
in the order that I did. Or maybe I'm wrong.
On 04/18/2012 03:26 PM, David C. Rankin wrote:
Here is what I have come up with from the additional posts here. I've updated
the patch to bug report 958. The link to the new patch is here:
http://bugs.pearsoncomputing.net/attachment.cgi?id=549
The code reads:
void K3bAudioEditorWidget::mousePressEvent( TQMouseEvent* e )
{
Range* r = findRangeEdge( e->pos(), &end );
m_draggedRange = 0;
m_draggedMarker = 0;
bool end;
if (r) {
m_draggedRange = r;
m_draggingRangeEnd = end;
setSelectedRange( r );
}
else {
r = findRange( e->pos() );
d->movedRange = r;
d->lastMovePosition = posToMsf( e->pos().x() );
setSelectedRange( r );
m_draggedMarker = findMarker( e->pos() );
}
TQFrame::mousePressEvent(e);
}
You guys that do the approval before Darrell submits, take a look and let us
know if it is good. I'm on to the next one.. else {
= findRange( e->pos() );
with the 'r' = of course :)
2:34 p.m.
New subject: [trinity-devel] 2nd Patch [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
On 18 Apr 2012, E. Liddell stated:
On Wed, 18 Apr 2012 15:52:26 -0500
"David C. Rankin" <drankinatty(a)suddenlinkmail.com> wrote:
> Range* r = findRangeEdge( e->pos(), &end );
> m_draggedRange = 0;
> m_draggedMarker = 0;
> bool end;
[...]
Are you sure that compiles? I would have assumed end
needed to be declared before
using a reference to it as an argument to findRangeEdge(), which was why I put things
in the order that I did. Or maybe I'm wrong.
Nope, you're right, that's the rule.
--
NULL && (void)
2:51 p.m.
New subject: [trinity-devel] 2nd Patch [was Re: [trinity-devel] k3b -bug ('K3bAudioEditorWidget::Range* r' previously declared here)]
On 04/18/2012 04:34 PM, Nix wrote:
On 18 Apr 2012, E. Liddell stated:
OK that's why I asked... fixed
attached..
--
David C. Rankin, J.D.,P.E.
On Wed, 18 Apr 2012 15:52:26 -0500
"David C. Rankin"<drankinatty(a)suddenlinkmail.com> wrote:
> Range* r = findRangeEdge( e->pos(),&end );
> m_draggedRange = 0;
> m_draggedMarker = 0;
> bool end;
[...]
Are you sure that compiles? I would have assumed
end needed to be declared before
using a reference to it as an argument to findRangeEdge(), which was why I put things
in the order that I did. Or maybe I'm wrong.
Nope, you're right, that's the rule.
4414
days inactive
4415
days old
12 comments
4 participants
participants (4)
-
Darrell Anderson -
David C. Rankin -
E. Liddell -
Nix