Обсуждение: warning in code while building on windows

1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
'isnull' : unreferenced local variable

It seems this variable is used only under macro USE_ASSERT_CHECKING,
so it is better to declare under this macro only.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila escribió:
> 1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
> 'isnull' : unreferenced local variable
> 
> It seems this variable is used only under macro USE_ASSERT_CHECKING,
> so it is better to declare under this macro only.

We have a macro for this, PG_USED_FOR_ASSERTS_ONLY.  This should silence
it; if the msvc stuff is not picking it up then maybe we need to tweak
the definition of the macro somehow.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 08/18/2013 05:08 PM, Alvaro Herrera wrote:
> Amit Kapila escribió:
>> 1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
>> 'isnull' : unreferenced local variable
>>
>> It seems this variable is used only under macro USE_ASSERT_CHECKING,
>> so it is better to declare under this macro only.
> We have a macro for this, PG_USED_FOR_ASSERTS_ONLY.  This should silence
> it; if the msvc stuff is not picking it up then maybe we need to tweak
> the definition of the macro somehow.
>


The macro is pretty gcc-specific, isn't it?

For MSVC we would probably need to surround the declaration with
something like these lines.
   #pragma warning(disable:4101)   #pragma warning(default:4101)

cheers

andrew

Andrew Dunstan escribió:
> 
> On 08/18/2013 05:08 PM, Alvaro Herrera wrote:
> >Amit Kapila escribió:
> >>1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
> >>'isnull' : unreferenced local variable
> >>
> >>It seems this variable is used only under macro USE_ASSERT_CHECKING,
> >>so it is better to declare under this macro only.
> >We have a macro for this, PG_USED_FOR_ASSERTS_ONLY.  This should silence
> >it; if the msvc stuff is not picking it up then maybe we need to tweak
> >the definition of the macro somehow.
> 
> The macro is pretty gcc-specific, isn't it?
> 
> For MSVC we would probably need to surround the declaration with
> something like these lines.
> 
>    #pragma warning(disable:4101)
>    #pragma warning(default:4101)

That seems pretty difficult to do with the position we've chosen for the
macro.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andrew Dunstan <andrew@dunslane.net> writes:
>> Amit Kapila escribi�:
>>> 1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
>>> 'isnull' : unreferenced local variable

> The macro is pretty gcc-specific, isn't it?

If that's the problem, why isn't Amit seeing a boatload of similar
warnings elsewhere?
        regards, tom lane

On Mon, Aug 19, 2013 at 9:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>>> Amit Kapila escribió:
>>>> 1>.\src\backend\utils\cache\relfilenodemap.c(213) : warning C4101:
>>>> 'isnull' : unreferenced local variable
>
>> The macro is pretty gcc-specific, isn't it?
>
> If that's the problem, why isn't Amit seeing a boatload of similar
> warnings elsewhere?

If I try to change other place similar to relfilenodemap.c, I am
getting similar warning. For example:

Original Code
--------------------
ExecMaterial()
{
..
/*            * Allocate a second read pointer to serve as the mark. We know it            * must have index 1, so
needn'tstore that.            */           int ptrno    PG_USED_FOR_ASSERTS_ONLY; 
           ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);          Assert(ptrno == 1); 
..
}

Modified Code
---------------------
ExecMaterial()
{
..
/*            * Allocate a second read pointer to serve as the mark. We know it            * must have index 1, so
needn'tstore that.            */           int ptrno    PG_USED_FOR_ASSERTS_ONLY; 

#ifdef USE_ASSERT_CHECKING
           ptrno = tuplestore_alloc_read_pointer(tuplestorestate,
node->eflags);          Assert(ptrno == 1); 
#endif
..
}

After above modification it gives below compilation error:
1>.\src\backend\executor\nodeMaterial.c(69) : warning C4101: 'ptrno' :
unreferenced local variable

Coming to original warning, changing the code as below removes warning:

RelidByRelfilenode()
{
..
#ifdef USE_ASSERT_CHECKING       if (assert_enabled)       {           Oid check;           bool isnull
PG_USED_FOR_ASSERTS_ONLY;          check = fastgetattr(ntp, Anum_pg_class_reltablespace,
RelationGetDescr(relation),                              &isnull);           Assert(!isnull && check == reltablespace); 
           check = fastgetattr(ntp, Anum_pg_class_relfilenode,
RelationGetDescr(relation),                              &isnull);           Assert(!isnull && check == relfilenode);
   } 
#endif

Moving isnull declaration inside if block resolves current compilation
warning, I think in any case it is better to declare inside if block
as it is used in that block only.

I think resolving the bigger problem such that it should not give
warning for such usage in MSVC is important, but can be dealt as a
separate thread/patch.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com