tinyxml usage in mingw-get CVS

[keithmarshall]

In the copy of tinyxml added by John E. to the mingw-get repository,
I see (with white space condensed) the following change from a copy
of the upstream source:

--- tinyxml-upstream/tinyxmlparser.cpp ...
+++ mingw-get/tinyxml/tinyxmlparser.cpp ...
@@ -22,6 +22,10 @@
 distribution.
 */

+/* Modified: JohnE, 2008-08-09
+ * Add parentheses to fix GCC -Wall warning
+ */
+
 #include <ctype.h>
 #include <stddef.h>

@@ -354,7 +358,7 @@
   }
   else
   {
-    while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
+    while ( (*p && IsWhiteSpace( *p )) || *p == '\n' || *p =='\r' )
       ++p;
   }

I don't see that warning, either with MinGW g++-3.4.5 or with Ubuntu
g++-4.2.4, but I guess it is present with later g++.  The original
logic is certainly (perhaps harmlessly) ambiguous, but surely the
intended expression would be:

  while( *p && (IsWhiteSpace( *p ) || *p == '\n' || *p == '\r') )

or, since IsWhiteSpace() is defined as:

  inline static bool IsWhiteSpace( char c )
  {
    return (isspace( (unsigned char) c ) || c == '\n' || c == '\r');
  }

(doesn't isspace() return true for '\n' and '\r' anyway?) wouldn't

  while( *p && IsWhiteSpace( *p ) )

suffice?

John, have you raised this issue with the tinyxml folks?

--

Regards,
Keith.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[Chris Sutcliffe-2]

> I don't see that warning, either with MinGW g++-3.4.5 or with Ubuntu
> g++-4.2.4, but I guess it is present with later g++.  The original
> logic is certainly (perhaps harmlessly) ambiguous, but surely the
> intended expression would be:

I don't see the warning with MinGW g++-4.4.0 either (I use tinyxml for
one of my other projects).

> John, have you raised this issue with the tinyxml folks?

I'm not sure how actively tinyxml is being developed.  The last
release was over 2 years ago, as was the last commit to CVS.

Chris

--
Chris Sutcliffe
http://emergedesktop.org

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[John E. / TDM]

Keith Marshall wrote:
> In the copy of tinyxml added by John E. to the mingw-get repository,
> I see (with white space condensed) the following change from a copy
> of the upstream source:
>  
*snip*

> +/* Modified: JohnE, 2008-08-09
> + * Add parentheses to fix GCC -Wall warning
> + */
> +
>  #include <ctype.h>
>  #include <stddef.h>
>
> @@ -354,7 +358,7 @@
>    }
>    else
>    {
> -    while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
> +    while ( (*p && IsWhiteSpace( *p )) || *p == '\n' || *p =='\r' )
>        ++p;
>    }
>
> I don't see that warning, either with MinGW g++-3.4.5 or with Ubuntu
> g++-4.2.4, but I guess it is present with later g++.

I don't know for certain when the fix became necessary, nor have I
compiled an unmodified version of TinyXML in quite a while.

>   The original
> logic is certainly (perhaps harmlessly) ambiguous, but surely the
> intended expression would be:
>
>   while( *p && (IsWhiteSpace( *p ) || *p == '\n' || *p == '\r') )
>
> or, since IsWhiteSpace() is defined as:
>
>   inline static bool IsWhiteSpace( char c )
>   {
>     return (isspace( (unsigned char) c ) || c == '\n' || c == '\r');
>   }
>
> (doesn't isspace() return true for '\n' and '\r' anyway?) wouldn't
>
>   while( *p && IsWhiteSpace( *p ) )
>
> suffice?
>  

I concur. The same conclusion was reached in discussion I witnessed for
another project.

> John, have you raised this issue with the tinyxml folks?
>  

I'll reply to Chris Sutcliffe's remark on this.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[John E. / TDM]

Chris Sutcliffe wrote:
>> John, have you raised this issue with the tinyxml folks?
>>    
>
> I'm not sure how actively tinyxml is being developed.  The last
> release was over 2 years ago, as was the last commit to CVS.
>  

Because of this, and because it's entirely uncritical, I haven't bothered.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[keithmarshall]

On Thursday 05 November 2009 00:31:14 John E. / TDM wrote:

> Chris Sutcliffe wrote:
> >> John, have you raised this issue with the tinyxml folks?
> >>    
> >
> > I'm not sure how actively tinyxml is being developed.  The last
> > release was over 2 years ago, as was the last commit to CVS.
> >  
>
> Because of this, and because it's entirely uncritical, I haven't
> bothered.

I just checked their developer forum, and their trackers.  There have
been three or four reports about this issue already; I've added a
comment to the latest forum report, with a pointer to this thread.  
Given the apparent lack of interest among the named developers,
beyond maintaining our own local patches, I see no point in pursuing
it further.

BTW, on their bug tracker I did notice one item which looks more
critical -- an invalid snprintf format specification ("%lf") which
Microsoft (and hence MinGW) deems as synonymous with "%Lf", but
which POSIX doesn't recognise, and could interpret as "%f".  It
affects the TiXmlAttribute::SetDoubleValue method, (which mingw-get
doesn't currently use, AFAICT); I'll patch it anyway, in our CVS.

--

Regards,
Keith.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[Earnie]

Quoting Keith Marshall <keithmarshall@...>:

> -    while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' )
> +    while ( (*p && IsWhiteSpace( *p )) || *p == '\n' || *p =='\r' )

This change seems wrong to me.  I think the change should be the  
following instead.  The reason I think this is the pointer *p needs  
tested for a value.

+    while ( *p && ( IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' ))

--
Earnie




------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[keithmarshall]

On Thursday 05 November 2009 18:21:55 Earnie Boyd wrote:
> > -    while ( *p && IsWhiteSpace( *p ) || *p == '\n' || *p =='\r'
> > ) +    while ( (*p && IsWhiteSpace( *p )) || *p == '\n' || *p
> > =='\r' )
>
> This change seems wrong to me.  I think the change should be the  
> following instead.  The reason I think this is the pointer *p
> needs tested for a value.
>
> +   while ( *p && (IsWhiteSpace( *p ) || *p == '\n' || *p =='\r' ))

Yes.  That's precisely the point I was making in my original post.  
However, because the checks for '\n' and '\r' are already performed
in IsWhiteSpace() anyway, (and seemingly redundant there too), it is
pointless repeating them here, so I plan to change it to just:

+   while ( *p && IsWhiteSpace( *p ) )

See also my earlier follow up.

--

Regards,
Keith.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[Earnie]

Quoting Keith Marshall <keithmarshall@...>:

> On Thursday 05 November 2009 00:31:14 John E. / TDM wrote:
>> Chris Sutcliffe wrote:
>> >> John, have you raised this issue with the tinyxml folks?
>> >>    
>> >
>> > I'm not sure how actively tinyxml is being developed.  The last
>> > release was over 2 years ago, as was the last commit to CVS.
>> >  
>>
>> Because of this, and because it's entirely uncritical, I haven't
>> bothered.
>
> I just checked their developer forum, and their trackers.  There have
> been three or four reports about this issue already; I've added a
> comment to the latest forum report, with a pointer to this thread.
> Given the apparent lack of interest among the named developers,
> beyond maintaining our own local patches, I see no point in pursuing
> it further.
>

SF does have a business method for a project takeover.  Maybe you want  
to pursue that, just to get the changes to the master source repository.

> BTW, on their bug tracker I did notice one item which looks more
> critical -- an invalid snprintf format specification ("%lf") which
> Microsoft (and hence MinGW) deems as synonymous with "%Lf", but
> which POSIX doesn't recognise, and could interpret as "%f".  It
> affects the TiXmlAttribute::SetDoubleValue method, (which mingw-get
> doesn't currently use, AFAICT); I'll patch it anyway, in our CVS.

Then we wouldn't have to have a fork.

--
Earnie



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr

[keithmarshall]

On Friday 06 November 2009 13:27:40 Earnie Boyd wrote:
> SF does have a business method for a project takeover.  Maybe you
> want   to pursue that, just to get the changes to the master
> source repository.

I have enough on my plate, without taking on another project.

> Then we wouldn't have to have a fork.

Well, the TinyXML deployment strategy is for every client project to
simply drop a handful of source files into its own tree, and compile
it as part of the project; every such client is potentially a fork.

That said, my comment on their developer forum did elicit a response
from Lee Thomason, (the original author), so maybe there is hope for
matching changes in the master repository.

--

Regards,
Keith.

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
MinGW-dvlpr mailing list
MinGW-dvlpr@...
https://lists.sourceforge.net/lists/listinfo/mingw-dvlpr