Skip to content
Permalink
Browse files

Fix unused variable warning with clang on Mac

  • Loading branch information
dakcarto committed Oct 7, 2015
1 parent 9b54ed4 commit 8f04d22fc6c73071b0d5d794c807b1443789455b
Showing with 1 addition and 0 deletions.
  1. +1 −0 src/core/qgsvectorlayerundocommand.cpp
@@ -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() );

8 comments on commit 8f04d22

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 7, 2015

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

This comment has been minimized.

Copy link
Member Author

@dakcarto dakcarto replied Oct 7, 2015

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

This comment has been minimized.

Copy link
Contributor

@brushtyler brushtyler replied Oct 7, 2015

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

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 7, 2015

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

@m-kuhn

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 7, 2015

@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

This comment has been minimized.

Copy link
Member Author

@dakcarto dakcarto replied Oct 8, 2015

@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

This comment has been minimized.

Copy link
Member

@m-kuhn m-kuhn replied Oct 8, 2015

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

This comment has been minimized.

Copy link
Member Author

@dakcarto dakcarto replied Oct 12, 2015

'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.
You can’t perform that action at this time.