unexpected result when calling method value() for negative integers

[gizero]

Hi,

I noticed a problem in the way value() is implemented in pylibconfig. I added a simple testcase to your tests.py which highlight the problem. See below the trivial change to test script.

If I'm getting it right, the point here is that value() will return a datum of the first type which makes lookupValue() return true. Looking at libconfig's implementation of cast operator to unsigned int, it appear that negative integers will make lookupValue() return true with a data value clipped to zero. Hence detecting data type by trials with lookupValue() will fail in this case.

Here is how libconfig overloads cast to unsigned int:

Setting::operator unsigned int() const throw(SettingTypeException)
{

  assertType(TypeInt);

  int v = config_setting_get_int(_setting);

  if(v < 0)

    v = 0;

  return(static_cast<unsigned int>(v));

}

Here is the patch to make the test suite fail for the given case:

diff --git a/tests/test.py b/tests/test.py
index 66b373c..4d8447a 100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -9,22 +9,29 @@ class PyLibConfigTest ( unittest.TestCase ):
         config = Config ()

         config.addString ( "", "test" )
         config.setValue ( "test", "value" )
+        config.addInteger ( "", "testInt" )
+        config.setValue ( "testInt", -4 )

         self.assert_ ( config.value ( "test" )[0] == "value" )
         self.assert_ ( config.value ( "test" )[1] == True )
         self.assert_ ( config.value ( "loose" )[1] == False )
         self.assert_ ( config.value ( "test_bool" )[0] == True )
         config.setValue( "test_bool", False )
+        self.assert_ ( config.value ( "testInt" )[0] == -4 )

         config.writeFile ( "test.conf" )

         self.assert_ ( os.path.exists ( "./test.conf" ) == True )

         config.readFile ( "test.conf" )

         self.assert_ ( config.value ( "test" )[0] == "value" )
         self.assert_ ( config.value ( "test" )[1] == True )
+        self.assert_ ( config.value ( "testInt" )[0] == -4 )

         os.remove ( "./test.conf" )
+
+if __name__ == '__main__':
+        unittest.main()

Am I getting it wrong?
Regards

[cnangel]

hi,
thanks, you could look up
https://github.com/cnangel/python-libconfig/blob/master/tests/test.py

2012/9/6 gizero notifications@github.com

Hi,

I noticed a problem in the way value() is implemented in pylibconfig. I
added a simple testcase to your tests.py which highlight the problem. See
below the trivial change to test script.

If I'm getting it right, the point here is that value() will return a
datum of the first type which makes lookupValue() return true. Looking at
libconfig's implementation of cast operator to unsigned int, it appear that
negative integers will make lookupValue() return true with a data value
clipped to zero. Hence detecting data type by trials with lookupValue()
will fail in this case.

Here is how libconfig overloads cast to unsigned int:
Setting::operator unsigned int() const throw(SettingTypeException)
{
assertType(TypeInt);

int v = config_setting_get_int(_setting);

if(v < 0)
v = 0;

return(static_cast(v));
}

Here is the patch to make the test suite fail for the given case:
diff --git a/tests/test.py b/tests/test.py
index 66b373c https://github.com/cnangel/python-libconfig/commit/66b373c..4d8447a
100644
--- a/tests/test.py
+++ b/tests/test.py
@@ -9,22 +9,29 @@ class PyLibConfigTest ( unittest.TestCase ):
config = Config ()

 config.addString ( "", "test" )
 config.setValue ( "test", "value" )

• config.addInteger ( "", "testInt" )

  config.setValue ( "testInt", -4 )

  self.assert_ ( config.value ( "test" )[0] == "value" )
  self.assert_ ( config.value ( "test" )[1] == True )
  self.assert_ ( config.value ( "loose" )[1] == False )
  self.assert_ ( config.value ( "test_bool" )[0] == True )
  config.setValue( "test_bool", False )

  self.assert_ ( config.value ( "testInt" )[0] == -4 )

  config.writeFile ( "test.conf" )

  self.assert_ ( os.path.exists ( "./test.conf" ) == True )

  config.readFile ( "test.conf" )

  self.assert_ ( config.value ( "test" )[0] == "value" )
  self.assert_ ( config.value ( "test" )[1] == True )

  self.assert_ ( config.value ( "testInt" )[0] == -4 )

  os.remove ( "./test.conf" )

  +if name == 'main':

  unittest.main()

Am I getting it wrong?
Regards

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2.

I love perl love me

[gizero]

hi cnangel,

looking at your last commit (d4c11aa), I think the swapping of tests in src/pylibconfig.cc will make the test case for negative integer pass, but I'm not sure it is conceptually correct.

My point here is that the lookup with an unsigned variable will never get executed since the more generic test for int will always win by design. I'm not a libconfig++ expert, but I poured my finger in its source code for a while now and, looking at how integer settings are stored, I see no distinction between signed and unsigned integers at lower levels (see CONFIG_TYPE_* macros and how they are used).

Hence, overloading the 'cast to unsigned int' operator with that sort of clipping to zero has not a clear purpose for me: should ask the libconfig++ guys... Calling such a cast, as lookupValue() does, sounds like 'I know it's potentially a signed value, but I want to look at it as unsigned, with the meaning of clipping it to zero if it's negative'. I can't even see any counterpart of this clipping behaviour in the libconfig's C API.

This behaviour is something that should not belong to the interface that python-libconfig is exporting to its users: it's even not suitable for distinguishing between signed and unsigned integer values IMO. Your purpose, here, if I get it right, is more like 'give me the value, whatever it is'. In the end, I'd personally drop the v_uint thing.

I'm curious what your opinion is here, since I might be overlooking something important.

Regards

[cnangel]

hi, gizero:
As you said, it really is a problem, i avoid it like this:
--- a/src/pylibconfig.cc
+++ b/src/pylibconfig.cc
@@ -45,14 +45,14 @@ public:
if ( config->lookupValue ( path, v_string ) )
return make_tuple ( v_string.c_str(), true );

•    unsigned int v_uint;
  

•    if ( config->lookupValue ( path, v_uint ) )
  

- return make_tuple ( v_uint, true );

     int v_int;
     if ( config->lookupValue ( path, v_int ) )
         return make_tuple ( v_int, true );

•    unsigned int v_uint;
  

•    if ( config->lookupValue ( path, v_uint ) )
  

•        return make_tuple ( v_uint, true );
  

  • bool v_bool;
    if ( config->lookupValue ( path, v_bool ) )
    return make_tuple ( v_bool, true );

Do you have better suggestions? [?]

2012/9/10 gizero notifications@github.com

hi cnangel,

looking at your last commit (d4c11aahttps://github.com/cnangel/python-libconfig/commit/d4c11aa94c666b44e799020a4099b3280bd6a85f),
I think the swapping of tests in src/pylibconfig.cc will make the test case
for negative integer pass, but I'm not sure it is conceptually correct.

My point here is that the lookup with an unsigned variable will never get
executed since the more generic test for int will always win by design. I'm
not a libconfig++ expert, but I poured my finger in its source code for a
while now and, looking at how integer settings are stored, I see no
distinction between signed and unsigned integers at lower levels (see
CONFIG_TYPE_* macros and how they are used).

Hence, overloading the 'cast to unsigned int' operator with that sort of
clipping to zero has not a clear purpose for me: should ask the libconfig++
guys... Calling such a cast, as lookupValue() does, sounds like 'I know
it's potentially a signed value, but I want to look at it as unsigned, with
the meaning of clipping it to zero if it's negative'. I can't even see any
counterpart of this clipping behaviour in the libconfig's C API.

This behaviour is something that should not belong to the interface that
python-libconfig is exporting to its users: it's even not suitable for
distinguishing between signed and unsigned integer values IMO. Your
purpose, here, if I get it right, is more like 'give me the value, whatever
it is'. In the end, I'd personally drop the v_uint thing.

I'm curious what your opinion is here, since I might be overlooking
something important.

Regards

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-8419090.

I love perl love me

[gizero]

Hi cnangel,

Sorry for not making this clear, but my last comment was after the change
you attach here. It actually was a comment about the way you worked around
the problem. See below for a re-explanation of my concern.

On Mon, Sep 10, 2012 at 12:57 PM, Cnangel notifications@github.com wrote:

hi, gizero:
As you said, it really is a problem, i avoid it like this:
--- a/src/pylibconfig.cc
+++ b/src/pylibconfig.cc
@@ -45,14 +45,14 @@ public:
if ( config->lookupValue ( path, v_string ) )
return make_tuple ( v_string.c_str(), true );

• unsigned int v_uint;
• if ( config->lookupValue ( path, v_uint ) )

- return make_tuple ( v_uint, true );

int v_int;
if ( config->lookupValue ( path, v_int ) )
return make_tuple ( v_int, true );

• unsigned int v_uint;
• if ( config->lookupValue ( path, v_uint ) )
• return make_tuple ( v_uint, true );
  +

Are you sure this code here will ever have any effect?
My point here is that the lookup with an unsigned variable will never get
executed since the more generic test for int will always win by design. I'm
not a libconfig++ expert, but I poured my finger in its source code for a
while now and, looking at how integer settings are stored, I see no
distinction between signed and unsigned integers at lower levels (see
CONFIG_TYPE_* macros and how they are used).

I'd personally drop the v_uint thing here.

Regards

bool v_bool;

if ( config->lookupValue ( path, v_bool ) )
return make_tuple ( v_bool, true );

Do you have better suggestions? [?]

2012/9/10 gizero notifications@github.com

hi cnangel,

looking at your last commit (d4c11aa<
d4c11aa94c666b44e799020a4099b3280bd6a85f>),

I think the swapping of tests in src/pylibconfig.cc will make the test
case
for negative integer pass, but I'm not sure it is conceptually correct.

My point here is that the lookup with an unsigned variable will never
get
executed since the more generic test for int will always win by design.
I'm
not a libconfig++ expert, but I poured my finger in its source code for
a
while now and, looking at how integer settings are stored, I see no
distinction between signed and unsigned integers at lower levels (see
CONFIG_TYPE_* macros and how they are used).

Hence, overloading the 'cast to unsigned int' operator with that sort of
clipping to zero has not a clear purpose for me: should ask the
libconfig++
guys... Calling such a cast, as lookupValue() does, sounds like 'I know
it's potentially a signed value, but I want to look at it as unsigned,
with
the meaning of clipping it to zero if it's negative'. I can't even see
any
counterpart of this clipping behaviour in the libconfig's C API.

This behaviour is something that should not belong to the interface that
python-libconfig is exporting to its users: it's even not suitable for
distinguishing between signed and unsigned integer values IMO. Your
purpose, here, if I get it right, is more like 'give me the value,
whatever
it is'. In the end, I'd personally drop the v_uint thing.

I'm curious what your opinion is here, since I might be overlooking
something important.

Regards

—
Reply to this email directly or view it on GitHub<
https://github.com/cnangel/python-libconfig/issues/2#issuecomment-8419090>.

I love perl love me

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-8420121.