info
Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
No, Really -- Regression in c_readme
11 views
Skip to first unread message
Paul S Person
Jun 6, 2019, 4:36:56 PM6/6/19
to
OK, this one appears to be real.
When I compiled the latest batch of Jiri's changes, I found myself
faced with 19 additional diffs in c_readme for PS, all of them in the
index. They are very strange: they appear to be pagination errors /in
the index/, and a brief check with wdw showed that the first item
misplaced by our wgml was moved because the metrics shows that it
should be. Which, of course, means that the problem lies elsewhere.
Synching with my last change (38048) reversed this, and then moving
forward identified Jiri's change 38061, which is described as:
correct some type mismatches
use new character classification macros
Synching back from 38061 to change 38060 showed only the single
expected diff (the date and time line).
This table summarizes the effect of change 38061:
File Device 38060 38061
c_readme PS 1 20
c_readme WHELP 0 2
f_readme PS 1 1
f_readme WHELP 0 0
cguide PS 299 310
cguide WHELP 124 124
Note that 1 diff is expected for PS, because PS files include the date
and time, which will differ between any two versions.
This has already cost one day of effort on implementing wgml. Figuring
out what Jiri's change 38061 got wrong will take who can say how long.
If this is allowed to continue, then wgml is over because I cannot
make any progress on wgml if my time is stolen by the need to correct
Jiri's errors. Although this is very surprising as his current changes
(when examined) appear to be cosmetic, and a great many have been made
with no problems, it is also very discouraging.
On the bright side, I was impressed with Perforce's ability, even for
someone using P4Win, to successfully identify the change at fault so
easily.
--
"I begin to envy Petronius."
"I have envied him long since."
When I compiled the latest batch of Jiri's changes, I found myself
faced with 19 additional diffs in c_readme for PS, all of them in the
index. They are very strange: they appear to be pagination errors /in
the index/, and a brief check with wdw showed that the first item
misplaced by our wgml was moved because the metrics shows that it
should be. Which, of course, means that the problem lies elsewhere.
Synching with my last change (38048) reversed this, and then moving
forward identified Jiri's change 38061, which is described as:
correct some type mismatches
use new character classification macros
Synching back from 38061 to change 38060 showed only the single
expected diff (the date and time line).
This table summarizes the effect of change 38061:
File Device 38060 38061
c_readme PS 1 20
c_readme WHELP 0 2
f_readme PS 1 1
f_readme WHELP 0 0
cguide PS 299 310
cguide WHELP 124 124
Note that 1 diff is expected for PS, because PS files include the date
and time, which will differ between any two versions.
This has already cost one day of effort on implementing wgml. Figuring
out what Jiri's change 38061 got wrong will take who can say how long.
If this is allowed to continue, then wgml is over because I cannot
make any progress on wgml if my time is stolen by the need to correct
Jiri's errors. Although this is very surprising as his current changes
(when examined) appear to be cosmetic, and a great many have been made
with no problems, it is also very discouraging.
On the bright side, I was impressed with Perforce's ability, even for
someone using P4Win, to successfully identify the change at fault so
easily.
--
"I begin to envy Petronius."
"I have envied him long since."
Jiri Malak
Jun 6, 2019, 7:12:02 PM6/6/19
to
I am sorry that your are feeling this way.
Difference is probably caused by fixing find_symvar_l() function in
gsymvar.c.
But I don't think that old code was OK.
If you want old behaviour then revert this change back.
Jiri
Dne 6.6.2019 v 22:36 Paul S Person napsal(a):
Difference is probably caused by fixing find_symvar_l() function in
gsymvar.c.
But I don't think that old code was OK.
If you want old behaviour then revert this change back.
Jiri
Dne 6.6.2019 v 22:36 Paul S Person napsal(a):
Paul S Person
Jun 6, 2019, 9:04:48 PM6/6/19
to
On Fri, 7 Jun 2019 01:11:51 +0200, Jiri Malak <malak...@gmail.com>
wrote:
>I am sorry that your are feeling this way.
You are sorry that I want to get wgml done without constantly having
to correct bugs introduced by someone else?
>Difference is probably caused by fixing find_symvar_l() function in
>gsymvar.c.
The only change I find there is
< while( !*p && isdigit(*p) ) {
-> while( my_isdigit( *p ) ) {
using P4WinDiff. This should have no effect on anything, given
#define my_isdigit( p ) isdigit( (unsigned char)(p) )
But I have checked all the files in change 38061 and everything is
just as innocuous, yet the degredation in behavior from 38060 to 38061
is quite clear.
>But I don't think that old code was OK.
>If you want old behaviour then revert this change back.
Maybe you could find the /actual/ change so I can see what you were
trying to do. It may only need a tweak, who can say?
wrote:
>I am sorry that your are feeling this way.
to correct bugs introduced by someone else?
>Difference is probably caused by fixing find_symvar_l() function in
>gsymvar.c.
< while( !*p && isdigit(*p) ) {
-> while( my_isdigit( *p ) ) {
using P4WinDiff. This should have no effect on anything, given
#define my_isdigit( p ) isdigit( (unsigned char)(p) )
But I have checked all the files in change 38061 and everything is
just as innocuous, yet the degredation in behavior from 38060 to 38061
is quite clear.
>But I don't think that old code was OK.
>If you want old behaviour then revert this change back.
trying to do. It may only need a tweak, who can say?
Jiri Malak
Jun 7, 2019, 1:13:18 AM6/7/19
to
I was trying to do code more portable to be compilable on 64-bit
platforms and by various compilers.
If you don't like my change simple remove it.
Because Perforce is not suitable for open development I can not change
code more suitable way, by example do pull request that you will be able
review my changes.
Simply I will not do any change to OW 1.9 code that you will be happy.
Jiri
Dne 7.6.2019 v 3:04 Paul S Person napsal(a):
platforms and by various compilers.
If you don't like my change simple remove it.
Because Perforce is not suitable for open development I can not change
code more suitable way, by example do pull request that you will be able
review my changes.
Simply I will not do any change to OW 1.9 code that you will be happy.
Jiri
Dne 7.6.2019 v 3:04 Paul S Person napsal(a):
Paul S Person
Jun 7, 2019, 1:01:00 PM6/7/19
to
On Fri, 7 Jun 2019 07:13:08 +0200, Jiri Malak <malak...@gmail.com>
wrote:
>I was trying to do code more portable to be compilable on 64-bit
>platforms and by various compilers.
This is part of the OpenWatcom code base. So far as I know, it need
only compile with the current OW distribution which, for /this/
server, is 1.9.
But do what you want, so long as every change compiles and none
produce regressions.
>If you don't like my change simple remove it.
Adults correct their mistakes.
Children do not
Which are you?
>Because Perforce is not suitable for open development I can not change
>code more suitable way, by example do pull request that you will be able
>review my changes.
Pretty much none of that makes sense to me.
If I, using P4Win, can determine which change introduced the problem
and which files it affected, why cannot you?
>Simply I will not do any change to OW 1.9 code that you will be happy.
I am not saying you should not change wgml code, merely that you
should "man up" and fix your mistake.
Perhaps you should start by building, for yourself, a framework that
will allow you to check diffs so you can see if your change causes a
regression and fix it before committing. So far, only one change has
been identified and, since the number of extra diffs is the same with
the head revision as with that change, it is very likely the only one.
And if you /want/ to actually improve wgml, there are non-diff tasks
that will need to be done to make it usable in the build system.
wrote:
>I was trying to do code more portable to be compilable on 64-bit
>platforms and by various compilers.
only compile with the current OW distribution which, for /this/
server, is 1.9.
But do what you want, so long as every change compiles and none
produce regressions.
>If you don't like my change simple remove it.
Children do not
Which are you?
>Because Perforce is not suitable for open development I can not change
>code more suitable way, by example do pull request that you will be able
>review my changes.
If I, using P4Win, can determine which change introduced the problem
and which files it affected, why cannot you?
>Simply I will not do any change to OW 1.9 code that you will be happy.
should "man up" and fix your mistake.
Perhaps you should start by building, for yourself, a framework that
will allow you to check diffs so you can see if your change causes a
regression and fix it before committing. So far, only one change has
been identified and, since the number of extra diffs is the same with
the head revision as with that change, it is very likely the only one.
And if you /want/ to actually improve wgml, there are non-diff tasks
that will need to be done to make it usable in the build system.
Paul S Person
Jun 7, 2019, 1:52:41 PM6/7/19
to
On Fri, 7 Jun 2019 07:13:08 +0200, Jiri Malak <malak...@gmail.com>
wrote:
>I was trying to do code more portable to be compilable on 64-bit
>platforms and by various compilers.
I should say this:
Most of your changes are a matter of indifference to me. Enjoy!
Replacing "uint8_t" with "unsigned char", however, I do not like
because of the /semantic/ difference: "uint8_t" is an /integer/, while
"unsigned char" is a character, and only very rarely are they mixed.
Some changes are improvements, such as removing superfluous include
file lines, making variable names clearer, or making conditionals
explicit.
Some are, perhaps, premature, particularly typedefs for simple
integers, since the code is not finished and there is /no/ guarantee
other will even be aware of them, never mind remember to use them.
wrote:
>I was trying to do code more portable to be compilable on 64-bit
>platforms and by various compilers.
Most of your changes are a matter of indifference to me. Enjoy!
Replacing "uint8_t" with "unsigned char", however, I do not like
because of the /semantic/ difference: "uint8_t" is an /integer/, while
"unsigned char" is a character, and only very rarely are they mixed.
Some changes are improvements, such as removing superfluous include
file lines, making variable names clearer, or making conditionals
explicit.
Some are, perhaps, premature, particularly typedefs for simple
integers, since the code is not finished and there is /no/ guarantee
other will even be aware of them, never mind remember to use them.
Paul S Person
Jun 7, 2019, 8:11:17 PM6/7/19
to
On Thu, 06 Jun 2019 13:36:27 -0700, Paul S Person
<pspe...@ix.netcom.invalid> wrote:
<context>
It occurred to me that, if I could synch to 38060 and then add each
file in turn from 38061, I could probably find out which one was
causing the problem.
And it worked. The file was gsymnar.c and the function was indeed
find_symvar_l(), as Jiri suggested, but he referenced a major change
and a problem with the code. So he changed
while( !*p && isdigit(*p) ) {
into
while( my_isdigit( *p ) ) {
and changing that into
while( !*p && my_isdigit(*p) ) {
fixed the problem. This /was/ a substantive change; too bad it wasn't
checked for regression.
Note to Jiri: so far as I can tell, the rules for /local/ symbols are:
1) the auto symbols (*, 1, 2 etc) have a scope that is limited to the
currrent macro
2) symbols created explicitly ("*fred=3" for example) have a scope
that the enire macro inclusion chain up to the first /file/; in
effect, the value of the symbol is inherited from the /last/ macro (or
the first file) that defines it unless overridden by the current macro
3) all symbols consisting entirely of numbers are treated as auto
symbols, whether they were created explicitly or not
This code supposed to be trapping all symbols consisting entirely of
numbers so that no attempt is made to find them in the local
dictionary chain.
After looking at it, I have come to the conclusion that you are
correct. Interestingly, the obvious change (changing "!*p" to "*p"
both here and the line below it in the file (which actually searches
the local dictionary chain) reduces the number of diffs to 3 (!) but
also produces 22 unfreed memory blocks and a claim that (Ox)264 chunks
are unfreed. Also, 33 pages appear to be missing. Very odd.
Still, I suppose this counts as progress of a sort. The missing pages
appear to include a GRAPHIC. I suppose I had better see what is going
on, as the problem you introduced is gone and this one could be mine,
or it could simply have been uncovered by fixing yours.
<pspe...@ix.netcom.invalid> wrote:
<context>
>When I compiled the latest batch of Jiri's changes, I found myself
>faced with 19 additional diffs in c_readme for PS, all of them in the
>index. They are very strange: they appear to be pagination errors /in
>the index/, and a brief check with wdw showed that the first item
>misplaced by our wgml was moved because the metrics shows that it
>should be. Which, of course, means that the problem lies elsewhere.
>
>Synching with my last change (38048) reversed this, and then moving
>forward identified Jiri's change 38061, which is described as:
>
> correct some type mismatches
> use new character classification macros
>
>Synching back from 38061 to change 38060 showed only the single
>expected diff (the date and time line).
<snippo>
>faced with 19 additional diffs in c_readme for PS, all of them in the
>index. They are very strange: they appear to be pagination errors /in
>the index/, and a brief check with wdw showed that the first item
>misplaced by our wgml was moved because the metrics shows that it
>should be. Which, of course, means that the problem lies elsewhere.
>
>Synching with my last change (38048) reversed this, and then moving
>forward identified Jiri's change 38061, which is described as:
>
> correct some type mismatches
> use new character classification macros
>
>Synching back from 38061 to change 38060 showed only the single
>expected diff (the date and time line).
It occurred to me that, if I could synch to 38060 and then add each
file in turn from 38061, I could probably find out which one was
causing the problem.
And it worked. The file was gsymnar.c and the function was indeed
find_symvar_l(), as Jiri suggested, but he referenced a major change
and a problem with the code. So he changed
while( !*p && isdigit(*p) ) {
while( my_isdigit( *p ) ) {
while( !*p && my_isdigit(*p) ) {
fixed the problem. This /was/ a substantive change; too bad it wasn't
checked for regression.
Note to Jiri: so far as I can tell, the rules for /local/ symbols are:
1) the auto symbols (*, 1, 2 etc) have a scope that is limited to the
currrent macro
2) symbols created explicitly ("*fred=3" for example) have a scope
that the enire macro inclusion chain up to the first /file/; in
effect, the value of the symbol is inherited from the /last/ macro (or
the first file) that defines it unless overridden by the current macro
3) all symbols consisting entirely of numbers are treated as auto
symbols, whether they were created explicitly or not
This code supposed to be trapping all symbols consisting entirely of
numbers so that no attempt is made to find them in the local
dictionary chain.
After looking at it, I have come to the conclusion that you are
correct. Interestingly, the obvious change (changing "!*p" to "*p"
both here and the line below it in the file (which actually searches
the local dictionary chain) reduces the number of diffs to 3 (!) but
also produces 22 unfreed memory blocks and a claim that (Ox)264 chunks
are unfreed. Also, 33 pages appear to be missing. Very odd.
Still, I suppose this counts as progress of a sort. The missing pages
appear to include a GRAPHIC. I suppose I had better see what is going
on, as the problem you introduced is gone and this one could be mine,
or it could simply have been uncovered by fixing yours.
Paul S Person
Jun 8, 2019, 8:36:21 AM6/8/19
to
On Fri, 07 Jun 2019 17:10:44 -0700, Paul S Person
A final note:
this was not, in fact, progress, as errors were preventing our file
from actually being produced and so the large diff was, in fact, the
entire document.
However, this has now been fixed by adding a test to not search for
auto local symbol * anywhere except the first local symbol dictionary.
And now I am back where I was before.
Jiri -- if you are doing a simple search-and-replace to enhance
portability or whatever, then /do/ a simple search-and-replace. Don't
omit other elements of a conditional even if it looks wrong. Fix it,
report it, or leave it alone. Please.
this was not, in fact, progress, as errors were preventing our file
from actually being produced and so the large diff was, in fact, the
entire document.
However, this has now been fixed by adding a test to not search for
auto local symbol * anywhere except the first local symbol dictionary.
And now I am back where I was before.
Jiri -- if you are doing a simple search-and-replace to enhance
portability or whatever, then /do/ a simple search-and-replace. Don't
omit other elements of a conditional even if it looks wrong. Fix it,
report it, or leave it alone. Please.
0 new messages