[Dune] [Dune-Commit] dune-common r6628 - in trunk/dune/common: parallel test

[Oliver Sander]

Hi Christoph,
I really appreciate your effort, but in many cases your use of DUNE_UNUSED
is not correct.  Let me discuss your patch to indicessyncer.hh as an example

>
> Modified: trunk/dune/common/parallel/indicessyncer.hh
> ===================================================================
> --- trunk/dune/common/parallel/indicessyncer.hh	2012-04-16 17:54:03 UTC (rev 6627)
> +++ trunk/dune/common/parallel/indicessyncer.hh	2012-04-17 10:04:47 UTC (rev 6628)
> @@ -7,6 +7,7 @@
>   #include<dune/common/stdstreams.hh>
>   #include<dune/common/tuples.hh>
>   #include<dune/common/sllist.hh>
> +#include<dune/common/unused.hh>
>   #include<cassert>
>   #include<cmath>
>   #include<limits>
> @@ -513,7 +514,7 @@
>         RemoteIndexIterator riEnd  = remote->second.first->end();
>         RemoteIndexIterator rIndex = remote->second.first->begin();
>         GlobalIndexIterator gIndex = global->second.begin();
> -      GlobalIndexIterator gEnd   = global->second.end();
> +      GlobalIndexIterator gEnd DUNE_UNUSED = global->second.end();

gEnd is really there without any reason at all.  Instead of marking it
with DUNE_UNUSED you should just throw it out.

>         IndexIterator       index  = indexSet.begin();
>
>         assert(rIndex==riEnd || gIndex != global->second.end());
> @@ -671,7 +672,7 @@
>
>         // Count the remote indices we know.
>         for(ValidIterator valid = collIter.begin(); valid != end; ++valid){
> -	++knownRemote;
> +        ++knownRemote;

Please avoid these whitespace changes.  If you need to do them please
apply them in a separate commit.

>         }
>
>         if(knownRemote>0){
> @@ -806,7 +807,7 @@
>       }
>
>       // Exchange indices with each neighbour
> -    const RemoteIterator rend = remoteIndices_.end();
> +    const RemoteIterator rend DUNE_UNUSED = remoteIndices_.end();
>

Again, kick out this variable, because it fulfills no purpose.
If people read DUNE_UNUSED they will think that the variable fulfills
some magic purpose that the compiler cannot figure out.

>
>   	if(iterators->second.isNotAtEnd()&&  iterators->second.isOld()
> @@ -1099,7 +1098,7 @@
>         // remote index list
>         SLList<std::pair<int,Attribute>  >  sourceAttributeList;
>         sourceAttributeList.push_back(std::make_pair(source,Attribute(sourceAttribute)));
> -      bool foundSelf=false;
> +      bool foundSelf DUNE_UNUSED = false;

Here foundSelf is actually used, but only in an assertion (hence if NDEBUG
is not set).  Hence stating that it is UNUSED is wrong.  If you want to
avoid warnings when compiling with optimization please wrap the declaration
of foundSelf in #ifndef NDEBUG .. #endif

Please review your recent DUNE_USED patches in the light of these comments.

Thank you,
Oliver

>         Attribute myAttribute=Attribute();
>
>         // Unpack the remote indices
>