My answer to these patch with some concerns, I'd appreciate if others could add opinion.
Begin forwarded message: > From: Viktor Szakáts <harbour...@syenar.hu> > Date: 2010 January 2 18:28:52 CET > To: Xavi <jara...@gmail.com> > Subject: Re: [Harbour] hbwin wapi_GetLastError() issue. > > Hi Xavi, > > On 2010 Jan 2, at 17:47, Xavi wrote: > >> Viktor, >> >> I have implemented the support in wapi_* for Windows API LastError. >> I think that I should do the job of updating the repository, thank you very >> much for doing before, I hope to consult you my doubts. >> >> Does the ChangeLog file is automatically updated with the Message cangelog >> or should we update it explicitly? > > You need to modify it manually. Please read doc/howtosvn.txt. > >> I installed the TortoiseSVN and prepared the following update, I hope it's >> correct. >> Please, could you grant acces to ID jarabal in sourceforge.net >> >> Message cangelog .- >> >> * contrib/hbwin/hbwapi.h >> + added wapi_StoreLastError() declaration. > > Since this is a low-level hbwin specific function > (or rather subsystem), please name it hbwin_StoreLastError() > and place implementation in new file win_err.c. > >> * contrib/hbwin/wapi_winbase.c >> * moved wapi_SetLastError() and wapi_GetLastError() >> to wapi_winbase_mutex.c > > Please leave them in wapi_winbase.c. Each wrapper > should stay in the source which implements a given > set of functions, for WAPI_SET/GETLASTERROR() this is > wapi_winbase.c, since Set/GetLastError() Windows API > functions are declared in winbase.h. See: > > http://msdn.microsoft.com/en-us/library/ms679360(VS.85).aspx > >> * contrib/hbwin/wapi_winbase_mutex.c >> + Added support for Windows API LastError in ST/MT mode. >> All functions wapi_* that can be used GetLastError() >> should be stored the value using wapi_StoreLastError(). >> wapi_GetLastError() return the stored value by threads >> before direct value of GetLastError(). > > Maybe I'm missing something, but I don't see why this needs > to be made that complicated. Windows already stores lasterror > in thread-local storage. Unless someone wants to use threads > created by non-Harbour means, storing lasterror for all > CPU threads inside every Harbour threads seems unnecessary. > > Code also uses a 'goto', which should IMO be replaced by > some other coding method. > >> + Added new PRG function .- >> nSizeThrs := wapi_HB_WorkSpaceSLE( [nSizeIncThrs] ) >> By default wapi_StoreLastError() stores and increases every 256 threads, >> can be increased, view or cancel the storage with nSizeIncThrs parameter. >> nSizeIncThrs == -1 cancel the storage for debugging tests. >> nSizeIncThrs == 0 or nil returns the current value. >> This value can be increase with nSizeIncThrs greater than current value. > > Should be named WIN_WORKSPACESLE() although IMO > this is the sort of low-level detail which should work > automatically without any .prg function which > requires programmers attention. 999 out of 1000 programmer > will not understand what is this, even less ppl > will know how and when to use it. I suggest to delete it > and simply set some good defaults. > > Missing from the patch is setting lasterror from all > such existing WAPI_* wrappers which may also set lasterror. > > Brgds, > Viktor > _______________________________________________ Harbour mailing list (attachment size limit: 40KB) Harbour@harbour-project.org http://lists.harbour-project.org/mailman/listinfo/harbour
