Hi Blair, Arkady
( > Blair, >> Arkady, >> BC> Blair )
- mywherex casts signed char to int, will overflow on 132 columns:
Arkady is right, this has to be fixed by explicitly using unsigned char.
- mywherex and mywherey do not process the page number byte at 40:62
which should be used as an index into a cursor word array at 40:50...
Arkady is right, but he did not mention that previous versions of
FreeCOM had the same bug in other implemementation...
> Does FreeCOM ever switch video pages? no. The point of getting rid of
> CONIO was to get rid of unused code...
Page switching is purely BIOS, it is not related to the CONIO
feature of using windowed output. So yes, it CAN happen that
FreeCOM is invoked while another page is active.
>> typedef unsigned char byte; /* eliminate troubles with sign extension */
>> #define MK_PTR(type,seg,ofs) \
>> ((type FAR*) MK_FP (seg, ofs)) /* safer edition of MK_FP */
>> typedef struct { byte col, row; } SCRPOS;
>> #define _scr_page (* MK_PTR (volatile const byte, 0, 0x462))
>> #define _scr_pos_array MK_PTR (volatile const SCRPOS, 0, 0x450)
>> unsigned mywherex (void) {
>> return _scr_pos_array [_scr_page].col + 1;
>> }
>> unsigned mywherey (void) {
>> return _scr_pos_array [_scr_page].row + 1;
>> }
I would recommend not to override byte and MK_FP locally.
The rest of the patch by Arkady looks like a very reasonable
implementation of mywherex and mywherey, although I wonder
why 0:0x4nn would be better than 0x40:nn - I think it is not.
It is better to EXPLICITLY use 0x40, to make it obvious to
people who read the source code that 40:xx is accessed. Even
if this means that FreeCOM will be 2.5 bytes larger ;-).
Blair, please include Arkady's mywherex and mywherey.
>> BC> +void outc(char c)
>> BC> + fflush( stdout );
>> BC> write( 1, &c, 1 );
>> Why not use fwrite() here?
> Because the code that's there works just as well.
I think you wanted to say void outc(char c) { fflush(stdout);
write(1,&c,1); } and that code does NOT work well. If you
flush stdout each time that you write a single character,
you get very slow output. My question is: Why do you need
fflush here? I would suggest to use outc only at places where
handle-oriented output is already used, and use fwrite (or,
if that exists, fputc / fputs), at places which use FILE output.
In the long run, it would be cool to get rid of FILE style
access at most places in FreeCOM, but that would be much work.
>> BC> +++ fcompl2.c 12 Jun 2006 04:55:42 -0000 1.4
>> BC> +#undef putchar
>> BC> +#define putchar outc
>> Why not use outc() explicitly?!
> Why?
Good question. fcompl2.c is only a single .c file, and #define
should normally go to header files. I mean: If you only want to
replace putchar by outc in a single .c file, and have to edit
that file for that anyway, it is better to be explicit instead
of using a local define. Just a matter of style of course.
>> BC> +++ cbreak.c 12 Jun 2006 04:55:42 -0000 1.3
>> BC> +void mycprintf( char *fmt, ... )
>> BC> + vsprintf( buffer, fmt, args );
>> BC> +#define cputs mycprintf
>> Then you in trouble, if there will be encountered "%" in string.
> But % is never encountered because cputs is only ever used to print
> \r\n. You would have known that if you read the code.
Then you have again a case of troublesome define. If cputs is only
used to print \r\n then you should better mycprintf("\r\n") directly.
Format % strings are a common source of troubles, and it is easy
to add trouble here: Somebody can insert a cputs with % later if he
is not careful. Again, better use mycprintf explicitly.
>> BC> +++ prprompt.c 12 Jun 2006 04:55:42 -0000 1.5
>> BC> +#define putchar outc
>>
>> Same question: why not use outc() explicitly, especially you anyway
>> change all occurrence of putchar()?!
> Because it's easier and a programmer will still understand what it does.
Actually it is not easier - just a single replace-all, and using
outc explicitly instead of redefining putchar keeps code MORE
readable for programmers. For example Java has no #define at all
to avoid all kinds of redefinition-caused readability issues...
>> BC> - case 'E': putchar("\33"); break; /* Decimal 27 */
>> BC> - case 'H': putchar("\10"); break; /* Decimal 8 */
>> BC> + case 'E': putchar('27'); break; /* Decimal 27 */
>> BC> + case 'H': putchar('8'); break; /* Decimal 8 */
>> Bug! You should omit apostrophes (use "outc(8)"), or add backslash
>> ("outc('\x8')").
I would vote for outc(8) and outc(27) here :-). Some compilers
will not accept "" enclosed STRING where single '' or numeric CHAR
should be passed, so there really is a bug here.
> Arkady, PLEASE stop reading my changes and trying to find fault with
> them, as I am the first to actually make changes to FreeCOM in a
> while. It'd be nice to get some thanks once in while.
Blair, it is very cool that you are improving FreeCOM, and I did
tell people that it is cool, off-list. However, you should be HAPPY
that Arkady is reading your code carefully, telling you about
remaining bugs. While your improved FreeCOM runs quite okay already,
the bugs found by Arkady still have to be fixed. Most of them are,
as explained above, real bugs. The cursor stuff means that FreeCOM
cursor handling breaks as soon as you use other pages or larger
text screen resolutions. Older versions of FreeCOM may have had the
same bug, but it is still a bug. It is particularily evil because
the char signedness is "hidden" - some compilers default to yes,
others to no, and you do not normally get an error message because
the compiler does not KNOW which of both you want. So it is good
to be explicit, as in Arkady's suggested patch.
All the outc / putchar / cputs / write / ... related comments by
Arkady might seem like ungrateful nitpicking at first glance but
he still has a point. The "bugs" only make FreeCOM slow and make
the code harder to read, but they are still worth to be fixed.
Better fix things now than encounter spurious bugs later when
others modify the code again and run into one or another less-
intuitive "define pitfall".
Of course Arkady sometimes suggests peephole improvements which
actually make code less readable, but it is still better to
appreciate his comments and improve all problematic issues which
he found. It is not mean personal / against you, it is only meant
to make FreeCOM even better.
That said, I think you remember the time when the devel/unstable
kernel received 1000s of lines of code changes, most of which
came from Jeremy (others from Lucho, Arkady, Bart...). Nobody
took the time to review all of those changes, although Arkady
did comment on quite a few of them. Now we have a real problem
because of LACK of comment and because too FEW people looked at
the same code: The unstable kernel has some real improvements
but also introduces some really experimental features, which
makes me stick to 2036 for stable. Of course I recommend "2037pre"
(I hope somebody can patch the unstable kernel to call itself
like that, to make it explicit that it IS more advanced than 2036)
for people who want more modern features and who can live with
a somehow increased probability of crashes / data corruption,
but it would be REALLY nice if some deus ex machina could read
the diff -ur between 2036 and 2037, which must be huge today.
I mean CALLING something stable is bad. We should have learned
that from beta9sr2, which contains unstable freecom and unstable
kernel and unstable nlsfunc and in the end it makes like 50% of
all users think that FreeDOS is utterly broken while 10% for
which things do work (which have a diskette drive, avoiding the
2035-unstable-of-the-time-when-the-iso-was made (we must use new
version numbers more often!) drive numbering bug, and which did
figure out that the boot cd menudefault was what crashed them,
while selecting "HIMEM only" or "HIMEM, EMM386" will get them
a working installer, as long as they survive NLSFUNC) think that
FreeDOS has some fancy new features. You know that I recommend
to avoid the 50% disappointed users rather than making the 10%
happy with experimental features.
And, hey, ISO files can be large. Just include BOTH the stable
and the new-experimental versions, but make the stable versions
the ones which are active by default. For FreeCOM, I am unsure
about this. Jeremy and you have done good work in cleaning up
0.84pre, so you could actually call it a real 0.84... Thanks a
lot :-). Several aspects of FreeCOM now support LFN, and, in
general, 0.84 is almost as stable as 0.82pl3. Not completely,
though, we still have the "funky criterr message issues" and
the "access empty cdrom confuses FreeCOM", both of which only
affect 0.84 ...
> I didn't see anyone thank me for fixing some bugzilla bugs...
Bugzilla has no automated notification-to-the world. If you
wrote something like "dear FreeDOS-user list, please test my
new FreeCOM 0.84 update, which fixes the FreeCOM bugs number
X, Y and Z, of which X and Y were introduced earlier in 0.84
and Z is an old bad bug since 0.82; The descriptions of the
bugs are: X ... Y ... Z ... (summary lines)" ... then you
will definitely get the attention from a few faithful testers
who will try your new FreeCOM and write a cheerful message
to you or even to the list, saying, HECK YES, you DID fix those
old bugs, THANK YOU :-).
Which reminds me that you should update history.txt / changelog
and similar files in the cvs, as people rarely read the by-file
changelog inside CVS itself :-). Instead, most people will just
download the current binary zip or source tarball and will be
very interested in seeing some cool news in the history.txt file.
Thanks for updating FreeCOM, and please see criticism as a way
to improve FreeCOM even further, rather than as an annoyance.
Have a nice (summer, in the northern hemisphere :-)) day!
Eric
_______________________________________________
Freedos-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/freedos-devel
