Skip to content

Commit

Permalink
Fix unused variable warning with clang on Mac
Browse files Browse the repository at this point in the history
  • Loading branch information
@dakcarto
dakcarto committed Oct 7, 2015
1 parent 9b54ed4 commit 8f04d22
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions src/core/qgsvectorlayerundocommand.cpp
View file
Expand Up @@ -47,6 +47,7 @@ void QgsVectorLayerUndoCommandAddFeature::undo()
#ifdef QGISDEBUG
QgsFeatureMap::const_iterator it = mBuffer->mAddedFeatures.find( mFeature.id() );
Q_ASSERT( it != mBuffer->mAddedFeatures.end() );
Q_UNUSED( it );
#endif
mBuffer->mAddedFeatures.remove( mFeature.id() );

Expand Down

8 comments on commit 8f04d22

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8f04d22 Oct 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is QGISDEBUG defined but Q_ASSERTs are not evaluated? Shouldn't QGISDEBUG be undefined in that scenario anyway?

Just wondering if fixing the problem at another point would have a bigger effect, avoid having to fix things like this again in the future and avoid the useless extra call to find().

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is QGISDEBUG defined but Q_ASSERTs are not evaluated? Shouldn't QGISDEBUG be undefined in that scenario anyway?

I have no idea why the use of 'it' inside of the Q_ASSERT bool parameter isn't noticed by the compiler after expanding the macro. This is compiling with clang on Mac from inside Qt Creator 3.5.0, with -DQGISDEBUG=1, so the definition is there. Not sure how it would be undefined in this situation.

Just wondering if fixing the problem at another point would have a bigger effect, avoid having to fix things like this again in the future and avoid the useless extra call to find().

Not sure I understand what fix in another place would keep this warning from showing up. I think it is localized to the Q_ASSERT macro and its expansion (or lack thereof). Should expand to the following (which should contain the variable in the condition) after preprocessing (from /src/corelib/global/qglobal.h):

#define Q_ASSERT(cond) ((!(cond)) ? qt_assert(#cond,__FILE__,__LINE__) : qt_noop())

@brushtyler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the Q_ASSERT macro is like a noop when QT_NO_DEBUG is set (i.e. release).

I guess there's no way to avoid to put Q_UNUSED after the Q_ASSERT if the variable is unused out of the assert condition.

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8f04d22 Oct 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so the check should be for QT_NO_DEBUG instead of QGSDEBUG?

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8f04d22 Oct 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakcarto is the warning produced for release builds or debug builds? For debug builds it would probably be good to have QT_NO_DEBUG unset and have the checks performed. Just a random guess, is that compiled with RelWithDebInfo and that's an effect of this?

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m-kuhn wrote:

... Just a random guess, is that compiled with RelWithDebInfo and that's an effect of this?

Bingo! That was exactly the issue, since I am doing RelWithDebInfo builds. So, in the qgis2.11.0.cbp file produced during configure, it defines

<Target title="qgis_core">
   ...
   <Compiler>
      ...
      <Add option="-DQT_NO_DEBUG" />
      ...
      <Add option="-DQGISDEBUG=1" />

Looks like someone (@nyalldawson ?) ran into this before and used the Q_UNUSED workaround as well, but apparently for a variable used within a QgsDebugMsg() call, which is a bit more odd, in that that macro is not expanding to a noop. Of the 77 uses of #ifdef QGISDEBUG there are no others that have a nested Q_ASSERT.

Suggestions on the best fix here? Just #include <cassert> and use assert(...) instead?

@m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented on 8f04d22 Oct 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inclusion will just fix asserts in this file, no?

What about changing it to #ifndef QT_NO_DEBUG and

  • build with built type DEBUG
    or
  • Make sure that RelWithDebInfo undefs QT_NO_DEBUG (or is having QT_NO_DEBUG defined the whole point of the RWDI configuration and there's some problem with this on mac)?

@dakcarto
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Fixed' in 307806a.

Make sure that RelWithDebInfo undefs QT_NO_DEBUG (or is having QT_NO_DEBUG defined the whole point of the RWDI configuration and there's some problem with this on mac)?

I think that is the point: just textual debug output, with no per-line testing.

Please sign in to comment.