On 04/18/2012 02:12 PM, E. Liddell wrote:
On Wed, 18 Apr 2012 13:25:43 -0500 "David C. Rankin" drankinatty@suddenlinkmail.com wrote:
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?
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, 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 :)