Changes in progress (was: Compiling for W...)

I think we both agree that the number of
changes I have made is getting large and
merging with existing CVS source will become
increasingly difficult as they grow. So I will
complete the changes I am in the process of
making, test them, and then look into how to
submit the changes.

A summary of the changes I am working on or
have completed follows.

- Elimination of compiler warnings from the
Windows build. There are 4 remaining that I
have intentionally left to look at more after I
understand the overall package better.

- Consistent const char **argv for TCL functions
and function pointers. This change followed
from the compiler warnings. A few may still
be hiding somewhere, but this is essentially
done. This was mostly accomplished by adding a
huge number of const keywords, in the
prototypes, definitions, and variables that
referencing the stings being passed.
However, there were a few instances
where the strings were really changed. In all
but one case, I did this the "right" way,
reworking the functions to leave the strings
alone instead of adding the overhead of an
allocate + copy. In that last case, there was
what seemed to be an unlikely situation that
was much easier to handle by making a copy.

- New timing facilities for Windows. The
replacement uses QueryPerformanceCounter()
and QueryPerformanceFrequency() for elapsed
time and GetProcessTimes() for kernel and
user times. The change should be transparent
to the caller, except for getting the right
times.

- libbu modifications
- A winutil file for utilities to work with
Windows. This file currently has only a
single function to change the '\' characters
in a Windows path specification to the '/'
characters used in Unix. This function
was created because I noticed this conversion
being done frequently with the same code
copied and pasted. I made it general in that
it can accept a size for the converted string
so it need not be null terminated and returns
the size of the string since that information
can essentially be found for free and may
be of use to the caller. The ability to
work with strings lacking a null termination
can be useful for both performance reasons
and to allow a string to remain a
const char *. I generally perfer to keep
track of string sizes myself to avoid things
like strlen().

- enhanced bu_bomb() to allow variable
arguments in message.

- bu_vls upgrades. I noticed that in a
significant number of instances, the vls
facility is used to build a small string
that is then discarded. With the current
version, this will always require an
allocation and free. To avoid this, a small
buffer was added to the end of the existing
structure, and the init call points the
string pointer to this buffer. The remaining
bu_vls functions were modified to make
the change transparent to callers of bu_vls.
While making the changes, I noticed that
the bu_vls_trimspace function was implemented
inefficiently with multiple function calls to
process a single character. So this funciton
was rewritten as well.
- Improved efficiency in help messages. The
few instances I had mentioned where the
bu_vls facility was used to create a copy
of a string that does not change were
only the tip of an iceberg. I am under water
now getting to the bottom. I have also
reformatted to some extent when making these
changes to conform with the coding standards
given because I had the alternatives of putting
in fixes that did not conform with the
standards, adding code that did conform that
was surronded by non-conforming code so that
it looked wrong, or fixing the wole thing to
conform. The conventions followed were as
in HACKING with two additions that I prefer.
Braces are always used to group loops and
ifs, with the exception of "else if". This
eases debugging and reduces the chances of
errors when another line is added to existing
code. Finally, the code is limited to a line
length of 80 characters, treating the tabs as
if they had their expanded size. This is
easier to read than lines wrapping around
and makes an occasional printing look much
better.

- Occasional efficiency improvements. One I
recall offhand is replacing something like
if (strlen(s) == 0) with if (*s == '\0') to
avoid the function call, which is doing noting
useful once it finds that the length is at
least 1.

- Project file fixes. There were problems
with the dependencies that caused a linker
warining that was strange and difficult to
identify. Also the post-build steps were
improved to echo information to the console
and to conditionally rebuild the destination
directories if they do not exist. One
additional thing I want to do in this iteration
is to separate the intermediate files (.obj,
etc. residing in directories Debug and
Release) into their own directory. This will
keep the directories holding the project
files clean, which will make it easier to move
the project from one computer to another.

- Others? I must be forgetting something here...

No, I have never used IRC. I will have to
look into that. I guess I still think in
batch mode. BTW, I still am not sure what
the differences between the Open Discussion
and Help forums are since all the posts seem to
be asking for some type of help, but replies to
this message probably belong in the Developers
forum, so I will look there.

I had hoped to complete changes I had been
working on and be ready to start testing them
more thoroughly after the long weekend, but
there was more work to do here than I had
anticipated. I found several more "char **argv"
prototypes that needed to be converted to "const
char **argv." This was due to incomplete
function and function pointer prototypes (e.g.,
bad_fun()) and external function declarations
within the .c file instead of headers. This
largly defeats the type checking provided by the
headers since a compiler will accept whatever
definition is given and the linker cannot do much
checking. (One exception is if a different
linkage convention is used if it leads to
different name decoration, but that is rarely
of much help.) It would be useful to change
the code to consistently use headers for all
external references, but I will not even think
about doing it until my current batch of chages
are integrated.

Others changes that I recall working on are
- Adding a bu_vls_memcpy() to assist with
working on unterminated strings or string
segments (which is useful when making the
char **argv's const)
- Closing some Windows handles when a process
is created. These had been left open, which
would prevent the release of the resources
associated with them until the program exits.
That is not terrible if it is done only a few
times, but the effects are cumulative.
- Allowing an EDITOR variable to select the
editor to use in the Windows version as the
comments describe and as is allowed in Unix.
(I prefer VIM over notepad for anything besides
trivial editing.) There is also the issue of
the 2-character end-of-line indicator in Windows
that does not appear to be addressed. I will
look into this further since I am still resolving
a const char related to the editing. BTW, that
is the last of them!
- Fixed the Windows version of printing a uid.
Windows returns a text name which was being
found OK but printed using a %d format. This
almost certainly was a copy and past from the
Unix version that was not fully modified.
- Cache the number of processors in Windows
since it cannot be dynamically varied.
- Cache the user name since it too is constant.
- Others? Probably. Should I think of them,
I will add them here for documentation purposes.

Adding to the list...
A new utility function for Windows,
bu_log_win32e() that functions much like
perror(), except it formats the message based
on Windows error codes obtained using
GetLastError() and takes a variable number
of arguments to use for formatting the message.
This will be useful when adding tests for
failure to some of the calls which do not
currently have them.

The changes mostly sound great. There's a lot of great work there that you've done that I'm anxious to get tested and integrated. From the description, it sounds like most of the changes are completely acceptible modifications to have applied.

The only one that raises a red flag is the bu_bomb() modification which will be highly dependant upon the nature of the change. In general (though not strictly speaking), bu_bomb() is merely a grace saving attempt to abort somewhat cleanly for a situation that would otherwise be a segmentation violation, bus error, or some other catastrophic condition that requires an immediate halt. As this includes conditions like being completely out of main or stack memory, bu_bomb() itself is implemented to function under minimal conditions. It doesn't require any additional stack allocation, for example, nor does it directly write to any memory. Even the call to fprintf() that it makes should probably not be using %s, though that is highly dependant upon the implementation of fprintf().

If you're interested in communicating over IRC (which I do highly recommend), there's a lot of info for getting started at http://irchelp.org. You basically install an IRC client and join the #brlcad channel on the irc.freenode.net network (port 6667). As for an IRC client, there are lots -- xchat is rather popular on windows, there are other options available too.

Batch mode submissions are workable, but they are certainly more "painful" as they can take a rather long time to review and integrate especially if there are any issues that require changes. Smaller patches (for first-time or external contributions) and smaller CVS commits (for developers) are considerably easier to peer review and validate. I'd rather see people commit directly to CVS whenever possible, but that's something that generally has to be discussed and set up via e-mail or IRC.

As for the forums, it's usually easier to just think of them as exclusive categories on developer and help. If it's developer related (whether asking for help or intended to be held as an open developer discussion or not), then it's developer forum discussion material. If it's asking for assistance, it belongs in the help forum. Otherwise, it's open discussion.

I am getting close to switching to something
more like a "testing mode" so I can verify that
things are working properly before I submit the
changes. This brings up the question of what
is available for black-box testing to verify
proper overall functionality. I saw the
benchmarks (although I have not looked at just
what they cover and test yet) but nothing
else. Is there any existing set of tests that
can be run? There are hundreds of basic units
written and waiting to be run. Right? Please
say yes...

bu_bomb() is interesting in a way to me because
it is a different way to program. When I
write something from scratch, I never allow a
called function terminate the program. That
is, should something bad happen, a function
will always return an error code which the
caller can use to determine how to proceed.
Termination is always by the function that is
the entry point (say main()), and unless there
is a good reason, there is only one exit point.
This gives the opportunity to attempt a recovery
somewhere or at least to produce a nice traceback
of messages from nested function calls if the
problem is fatal. The tradeoff is that every call that can fail needs to be tested, although I
personally think it is worthwhile. Most of the
calls I have seen so far to bu_bomb() are made
when something undesirable but not in itself
fatal occurs, such as failure of a memory
allocation or failure to open a file.

As far as the actual proposed change to bu_bomb()
it seems to be a fairly trivial change from
fprintf() to vfprintf().