Experimental Physics and
Industrial Control System

Andrew Johnson via Core-talk <[email protected]> · Thu, 19 Sep 2019 17:32:11 -0000

I see what you mean about viewing in-diff comments. Clicking one of the "Show diff comments" links in an existing comment helped me earlier, but older comments seem to have disappeared (at least while I have some new unsaved diff comments pending).

I withdraw my size_t suggestion, but see my new in-diff comments regarding the name.

Diff comments:

> diff --git a/modules/libcom/src/log/logClient.c b/modules/libcom/src/log/logClient.c
> index 99ee671..b2c9acc 100644
> --- a/modules/libcom/src/log/logClient.c
> +++ b/modules/libcom/src/log/logClient.c
> @@ -21,11 +21,9 @@
>  #include <string.h>
>  #include <stdio.h>
>  
> -#define epicsExportSharedSymbols

The definition of epicsExportSharedSymbols marks the boundary between "other DLLs" and "this DLL"; all "other DLL" symbols from EPICS headers (i.e. ones that use shareLib.h) must have been defined with the appropriate epicsShare decorations before the macro gets defined, after which all "this DLL" symbols should be defined (usually by including their headers). Since everything in libCom goes into the same DLL, the macro needs to be defined before any other libCom headers are included.

Microsoft's compilers seem to be able to recognize when some symbols get declared with the wrong decoration and will correct them without generating an error, but GCC (as used for MingW builds) doesn't do that. There does seem to be some kind of a limit to how many symbols the MS compiler can correct, and it also behaves differently with building for DEBUG or not. This is probably why you aren't seeing an error from your windows builds.

The epicsExport.h header is a bit of an anomaly which is hard to fix because it declares some essential things like the epicsExportAddress() macro and also defines epicsExportSharedSymbols. When you're just writing device support or record support this is convenient because it's rare that you need to call other routines in the same DLL, so Marty's simple rule was to make epicsExport.h the last include file. That doesn't work for the core DLLs though where we're often using symbols from both this and other DLLs.

Please put the definition of epicsExportSharedSymbols back here where it belongs.

>  #include "dbDefs.h"
>  #include "epicsEvent.h"
>  #include "iocLog.h"
> -#include "errlog.h"
>  #include "epicsMutex.h"
>  #include "epicsThread.h"
>  #include "epicsTime.h"
> diff --git a/modules/libcom/src/osi/Makefile b/modules/libcom/src/osi/Makefile
> index ecbf4c2..3028baf 100644
> --- a/modules/libcom/src/osi/Makefile
> +++ b/modules/libcom/src/osi/Makefile
> @@ -86,6 +86,7 @@ endif
>  
>  Com_SRCS += osdSock.c
>  Com_SRCS += osdSockAddrReuse.cpp
> +Com_SRCS += osdSockCountUnsentBytes.c

Why did you add all these new files instead of putting the routine in the existing osdSock.c files? I'm not saying that's wrong if you need to do that for each OS to get the right routine, but I don't see the need. Every new source file that you add does need to have the EPICS header added to it:

/*************************************************************************\
* EPICS BASE is distributed subject to a Software License Agreement found
* in file LICENSE that is included with this distribution.
\*************************************************************************/

I still don't like your routine name. Networking APIs have traditionally used the term "octet" instead of "byte" and my suggestion explicitly avoided using that word, as the various asynOctet types and routines do. The Asyn APIs were defined by Eric Norum whose experience dates back to when a byte could be 6, 7, 8 or even 9 bits wide, depending on what CPU you were using. https://en.wikipedia.org/wiki/Byte

How about
    int epicsSocketUnsentCount(SOCKET sock);
By moving the word Count to the end of the name I'm clarifying that it's being used as a noun and not a verb (a subtlety of English that may contradict German usage) — we don't want it to actually count the unsent octets, we just want to know what the result of the earlier count was.

>  Com_SRCS += osiSock.c
>  Com_SRCS += systemCallIntMech.cpp
>  Com_SRCS += epicsSocketConvertErrnoToString.cpp

-- 
https://code.launchpad.net/~dirk.zimoch/epics-base/+git/epics-base/+merge/372925
Your team EPICS Core Developers is subscribed to branch epics-base:7.0.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System