Merge ~info-martin-konrad/epics-base:clean-up-msi into ~epics-core/epics-base/+git/epics-base:3.15

[Martin Konrad]

Further clean up to simplify code and replace manual memory management by leveraging C++ features (e.g. std::string, std::list instead of potentially unsafe fiddling with raw pointers).

[mdavidsaver (mdavidsaver)]

The MSI test still passes: https://travis-ci.org/mdavidsaver/epics-base/builds/479219104

There is nothing here which I don't like, so I'm inclined to merge this.

There is a more which could be done to eliminate the remaining explicit deletes.

1. The inputData struct is only ever instantiated in main(). So it could be made a local, and the inputCloseAllFiles() call moved to ~inputData.

2. Similarly for subInfo, which is also only instantiated in main().

3. subInfo::psubFile cries out for auto_ptr.

review: Approve

[Ralph Lange (ralph-lange)]

Looks good to me, too.

Re 3. It would feel somewhat weird to introduce a concept that immediately creates deprecation warnings. I know it's a catch 22, just saying.

[mdavidsaver (mdavidsaver)]

On 1/14/19 12:57 AM, Ralph Lange wrote:
> Looks good to me, too.
>
> Re 3. It would feel somewhat weird to introduce a concept that immediately creates deprecation warnings. I know it's a catch 22, just saying.

It feels weird that in 2019 we're still at c++98 with no timetable to moving forward.

[Andrew Johnson (anj)]

I'm very happy to see this work being done, thanks Martin. I haven't looked at the code yet and I don't know if/when I'll have time to do so, but since both Ralph and Michael have approved this at least in principle I don't think I need to.

Since msi gets built for host architectures only, its code doesn't have to meet the compiler limitations of our embedded targets. Of course the MSVC C++ compilers have their own limitations so I would like to see this code built against various versions of Visual C++ before merging, but I wouldn't be averse to removing the older versions of that compiler from what we support.

We should probably update and better document our set of pre-requisites anyway. The list in documentation/README.html is out of date in places, and while there are some versions listed on the APS website it would be better to have a definitive list in the source tree.

- Andrew

[Martin Konrad (info-martin-konrad)]

While making the modifications on this branch I was also wondering if we could relax the C++98 requirement for host-only applications like MSI. Some newer C++ features would in fact come quite handy. Initializer lists, range-based for loops, std::forward_list, const iterators, and the auto keyword are all C++03/11 features - not to mention move semantics and std::regex which might allow us to simplify the code for parsing significantly without adding any new dependencies.

Which C++ version do you think would be realistic?

BTW: I successfully built this branch with MSVC++ 14.16 (a.k.a. Visual Studio 2017 v15.9) - all MSI tests passed.

[Andrew Johnson (anj)]

Any kind of 'what should we be supporting' question should really go to tech-talk, explaining what the issues are and what software and targets would be affected. I'm not saying that *anyone* using EPICS has veto power over this kind of decision, but we should let the community know and allow them some input before we do something that might have effects we don't know about.

From the perspective of the APS Controls group I would want to be able to use the standard version of G++ that comes with RHEL-7.4 (or maybe 7.5 but I'd have to check with IT to see what the oldest RHEL version is that we have installed on our systems). Other groups may have other criteria, such as they need PVA and the latest AreaDetector to run IOCs on a Linux or Windows CPU that comes built into some commercial detector box whose manufacturer only provides <whatever>.

[Andrew Johnson (anj)]

Core group: Plans for C++11 support will be announced soon.

Back to MSI: We're considering replacing msi with a Perl version, Benjamin Franksen has offered, which would save having to maintain this complex code. However we don't expect it soon, and it might never happen in which case we will revisit this merge request (please don't delete this MR yet, we may add your test case as a TODO to msi.plt).

[Martin Konrad (info-martin-konrad)]

Any news on the Perl version? If we can't retire the C++ version, yet, I would appreciate if we could get this or the smaller version (https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+merge/361500) merged.

[Ben Franksen (bfrk)]

> Any news on the Perl version?

I may have been a bit hasty with my offer to re-write it in Perl. It is less a matter of time and effort and more one of motivation. Perl is fun for quick and dirty data mining, but doing serious reliable parsing with it is not (IME). C/C++ may not be ideal for this task but at least it is statically typed. Anyway, a rewrite in Perl will need serious real-life testing over an extended period before it can replace the existing code, so I see no good reason to hold back on merging your fixes.

[mdavidsaver (mdavidsaver)]

Andrew/Ralph, share we reconsider merging this work? (maybe against 7.0 if you have concerns wrt. 3.15)

[mdavidsaver (mdavidsaver)]

Having heard no objections, I'm going to merge this into 3.15.

[Andrew Johnson (anj)]

While merging 3.15 up to the 7.0 branch I noticed that Martin removed the inclusion of errlog.h and the call to errlogFlush() at the end of the main() routine. While there are no calls to errlog routines in the msi.cpp file, the macLib routines *do* call errlogPrintf() in the event that errors are detected, so the errlogFlush() is still necessary. I will restore it.

[Martin Konrad (info-martin-konrad)]

Good catch. Some day I will write a proper C++ wrapper around the EPICS logger that flushes automatically when it gets destroyed. I'm actually using something like that in some of my support modules: https://gist.github.com/mark0n/d36292ac2640b96c433892720bacd032. But I guess as long as we still have code using the old C API I guess I have to be careful...

diff --git a/src/ioc/dbtemplate/msi.cpp b/src/ioc/dbtemplate/msi.cpp
index 9a370e8..49d8611 100644
--- a/src/ioc/dbtemplate/msi.cpp
+++ b/src/ioc/dbtemplate/msi.cpp
@@ -10,6 +10,7 @@
 /* msi - macro substitutions and include */
 
 #include <string>
+#include <list>
 
 #include <stdlib.h>
 #include <stddef.h>
@@ -20,8 +21,6 @@
 
 #include <dbDefs.h>
 #include <macLib.h>
-#include <ellLib.h>
-#include <errlog.h>
 #include <epicsString.h>
 #include <osiFileName.h>
 #include <osiUnistd.h>
@@ -68,10 +67,10 @@ static void inputErrPrint(const inputData * const pvt);
 /* Module to read the substitution file */
 typedef struct subInfo subInfo;
 
-static void substituteOpen(subInfo **ppvt, char * const substitutionName);
+static void substituteOpen(subInfo **ppvt, const std::string& substitutionName);
 static void substituteDestruct(subInfo * const pvt);
-static int substituteGetNextSet(subInfo * const pvt, char **filename);
-static int substituteGetGlobalSet(subInfo * const pvt);
+static bool substituteGetNextSet(subInfo * const pvt, char **filename);
+static bool substituteGetGlobalSet(subInfo * const pvt);
 static const char *substituteGetReplacements(subInfo * const pvt);
 static const char *substituteGetGlobalReplacements(subInfo * const pvt);
 
@@ -86,7 +85,7 @@ static void makeSubstitutions(inputData * const inputPvt,
 
 /*Global variables */
 static int opt_V = 0;
-static int opt_D = 0;
+static bool opt_D = false;
 
 static char *outFile = 0;
 static int numDeps = 0, depHashes[MAX_DEPS];
@@ -97,23 +96,21 @@ int main(int argc,char **argv)
     inputData *inputPvt;
     MAC_HANDLE *macPvt;
     char *pval;
-    int  narg;
-    char *substitutionName = 0;
+    std::string substitutionName;
     char *templateName = 0;
-    int  i;
-    int  localScope = 1;
+    bool localScope = true;
 
     inputConstruct(&inputPvt);
     macCreateHandle(&macPvt, 0);
     while ((argc > 1) && (argv[1][0] == '-')) {
-        narg = (strlen(argv[1]) == 2) ? 2 : 1;
+        int narg = (strlen(argv[1]) == 2) ? 2 : 1;
         pval = (narg == 1) ? (argv[1] + 2) : argv[2];
 
         if (strncmp(argv[1], "-I", 2) == 0) {
             inputAddPath(inputPvt, pval);
         }
         else if (strcmp(argv[1], "-D") == 0) {
-            opt_D = 1;
+            opt_D = true;
             narg = 1; /* no argument for this option */
         }
         else if(strncmp(argv[1], "-o", 2) == 0) {
@@ -123,14 +120,14 @@ int main(int argc,char **argv)
             addMacroReplacements(macPvt, pval);
         }
         else if(strncmp(argv[1], "-S", 2) == 0) {
-            substitutionName = epicsStrDup(pval);
+            substitutionName = pval;
         }
         else if (strcmp(argv[1], "-V") == 0) {
             opt_V = 1;
             narg = 1; /* no argument for this option */
         }
         else if (strcmp(argv[1], "-g") == 0) {
-            localScope = 0;
+            localScope = false;
             narg = 1; /* no argument for this option */
         }
         else if (strcmp(argv[1], "-h") == 0) {
@@ -142,7 +139,7 @@ int main(int argc,char **argv)
         }
 
         argc -= narg;
-        for (i = 1; i < argc; i++)
+        for (int i = 1; i < argc; i++)
             argv[i] = argv[i + narg];
     }
 
@@ -170,16 +167,16 @@ int main(int argc,char **argv)
     if (argc == 2)
         templateName = epicsStrDup(argv[1]);
 
-    if (!substitutionName) {
+    if (substitutionName.empty()) {
         STEP("Single template+substitutions file");
         makeSubstitutions(inputPvt, macPvt, templateName);
     }
     else {
         subInfo *substitutePvt;
         char *filename = 0;
-        int isGlobal, isFile;
+        bool isGlobal, isFile;
 
-        STEPS("Substitutions from file", substitutionName);
+        STEPS("Substitutions from file", substitutionName.c_str());
         substituteOpen(&substitutePvt, substitutionName);
         do {
             isGlobal = substituteGetGlobalSet(substitutePvt);
@@ -213,14 +210,12 @@ int main(int argc,char **argv)
         } while (isGlobal || isFile);
         substituteDestruct(substitutePvt);
     }
-    errlogFlush();
     macDeleteHandle(macPvt);
     inputDestruct(inputPvt);
     if (opt_D) {
         printf("\n");
     }
     free(templateName);
-    free(substitutionName);
     return opt_V & 2;
 }
 

@@ -301,7 +296,6 @@ static void makeSubstitutions(inputData * const inputPvt,
         if (command) {
             char *pstart;
             char *pend;
-            char *copy;
             int  cmdind=-1;
             int  i;
 
@@ -334,16 +328,15 @@ static void makeSubstitutions(inputData * const inputPvt,
             /*skip quote and any trailing blanks*/
             while (*++p == ' ') ;
             if (*p != '\n' && *p != 0) goto endcmd;
-            copy = static_cast<char *>(calloc(pend-pstart + 1, sizeof(char)));
-            strncpy(copy, pstart, pend-pstart);
+            std::string copy = std::string(pstart, pend);
 
             switch(cmdind) {
             case cmdInclude:
-                inputNewIncludeFile(inputPvt,copy);
+                inputNewIncludeFile(inputPvt, copy.c_str());
                 break;
 
             case cmdSubstitute:
-                addMacroReplacements(macPvt,copy);
+                addMacroReplacements(macPvt, copy.c_str());
                 break;
 
             default:
@@ -351,7 +344,6 @@ static void makeSubstitutions(inputData * const inputPvt,
                 inputErrPrint(inputPvt);
                 abortExit(1);
             }
-            free(copy);
             expand = 0;
         }
 
@@ -370,21 +362,16 @@ endcmd:
 }
 

 typedef struct inputFile {
-    ELLNODE     node;
-    char        *filename;
+    std::string filename;
     FILE        *fp;
     int         lineNum;
 } inputFile;
 
-typedef struct pathNode {
-    ELLNODE     node;
-    char        *directory;
-} pathNode;
-
 struct inputData {
-    ELLLIST     inputFileList;
-    ELLLIST     pathList;
+    std::list<inputFile> inputFileList;
+    std::list<std::string> pathList;
     char        inputBuffer[MAX_BUFFER_SIZE];
+    inputData() { memset(inputBuffer, 0, sizeof(inputBuffer) * sizeof(inputBuffer[0])); };
 };
 
 static void inputOpenFile(inputData *pinputData, const char * const filename);
@@ -393,66 +380,49 @@ static void inputCloseAllFiles(inputData *pinputData);
 
 static void inputConstruct(inputData **ppvt)
 {
-    inputData *pinputData;
-
-    pinputData = static_cast<inputData *>(calloc(1, sizeof(inputData)));
-    ellInit(&pinputData->inputFileList);
-    ellInit(&pinputData->pathList);
-    *ppvt = pinputData;
+    *ppvt = new inputData;
 }
 
 static void inputDestruct(inputData * const pinputData)
 {
-    pathNode *ppathNode;
-
     inputCloseAllFiles(pinputData);
-    while ((ppathNode = (pathNode *) ellFirst(&pinputData->pathList))) {
-        ellDelete(&pinputData->pathList, &ppathNode->node);
-        free(ppathNode->directory);
-        free(ppathNode);
-    }
-    free(pinputData);
+    delete(pinputData);
 }
 

 static void inputAddPath(inputData * const pinputData, const char * const path)
 {
-    ELLLIST     *ppathList = &pinputData->pathList;
-    pathNode    *ppathNode;
     const char  *pcolon;
     const char  *pdir;
     size_t      len;
-    int         emptyName;
     const char  sep = *OSI_PATH_LIST_SEPARATOR;
 
     ENTER;
     pdir = path;
     /*an empty name at beginning, middle, or end means current directory*/
     while (pdir && *pdir) {
-        emptyName = ((*pdir == sep) ? 1 : 0);
+        bool emptyName = (*pdir == sep);
         if (emptyName) ++pdir;
 
-        ppathNode = (pathNode *) calloc(1, sizeof(pathNode));
-        ellAdd(ppathList, &ppathNode->node);
-
+        std::string directory;
         if (!emptyName) {
             pcolon = strchr(pdir, sep);
             len = (pcolon ? (pcolon - pdir) : strlen(pdir));
             if (len > 0)  {
-                ppathNode->directory = (char *) calloc(len + 1, sizeof(char));
-                strncpy(ppathNode->directory, pdir, len);
+                directory = std::string(pdir, len);
                 pdir = pcolon;
                 /*unless at end skip past first colon*/
                 if (pdir && *(pdir + 1) != 0) ++pdir;
             }
             else { /*must have been trailing : */
-                emptyName = 1;
+                emptyName = true;
             }
         }
 
         if (emptyName) {
-            ppathNode->directory = (char *) calloc(2, sizeof(char));
-            strcpy(ppathNode->directory, ".");
+            directory = ".";
         }
+
+        pinputData->pathList.push_back(directory);
     }
     EXIT;
 }
@@ -467,14 +437,14 @@ static void inputBegin(inputData * const pinputData, const char * const fileName
 
 static char *inputNextLine(inputData * const pinputData)
 {
-    inputFile   *pinputFile;
-    char        *pline;
+    std::list<inputFile>& inFileList = pinputData->inputFileList;
 
     ENTER;
-    while ((pinputFile = (inputFile *) ellFirst(&pinputData->inputFileList))) {
-        pline = fgets(pinputData->inputBuffer, MAX_BUFFER_SIZE, pinputFile->fp);
+    while (!inFileList.empty()) {
+        inputFile& inFile = inFileList.front();
+        char *pline = fgets(pinputData->inputBuffer, MAX_BUFFER_SIZE, inFile.fp);
         if (pline) {
-            ++pinputFile->lineNum;
+            ++inFile.lineNum;
             EXITS(pline);
             return pline;
         }
@@ -494,23 +464,21 @@ static void inputNewIncludeFile(inputData * const pinputData,
 
 static void inputErrPrint(const inputData *const pinputData)
 {
-    inputFile   *pinputFile;
-
     ENTER;
     fprintf(stderr, "input: '%s' at ", pinputData->inputBuffer);
-    pinputFile = (inputFile *) ellFirst(&pinputData->inputFileList);
-    while (pinputFile) {
-        fprintf(stderr, "line %d of ", pinputFile->lineNum);
+    const std::list<inputFile>& inFileList = pinputData->inputFileList;
+    std::list<inputFile>::const_iterator inFileIt = inFileList.begin();
+    while (inFileIt != inFileList.end()) {
+        fprintf(stderr, "line %d of ", inFileIt->lineNum);
 
-        if (pinputFile->filename) {
-            fprintf(stderr, " file %s\n", pinputFile->filename);
+        if (!inFileIt->filename.empty()) {
+            fprintf(stderr, " file %s\n", inFileIt->filename.c_str());
         }
         else {
             fprintf(stderr, "stdin:\n");
         }
 
-        pinputFile = (inputFile *) ellNext(&pinputFile->node);
-        if (pinputFile) {
+        if (++inFileIt != inFileList.end()) {
             fprintf(stderr, "  included from ");
         }
         else {
@@ -523,10 +491,9 @@ static void inputErrPrint(const inputData *const pinputData)
 

 static void inputOpenFile(inputData *pinputData, const char * const filename)
 {
-    ELLLIST     *ppathList = &pinputData->pathList;
-    pathNode    *ppathNode = 0;
-    inputFile   *pinputFile;
-    char        *fullname = 0;
+    std::list<std::string>& pathList = pinputData->pathList;
+    std::list<std::string>::iterator pathIt = pathList.end();
+    std::string fullname;
     FILE        *fp = 0;
 
     ENTER;
@@ -534,24 +501,19 @@ static void inputOpenFile(inputData *pinputData, const char * const filename)
         STEP("Using stdin");
         fp = stdin;
     }
-    else if ((ellCount(ppathList) == 0) || strchr(filename, '/')){
+    else if (pathList.empty() || strchr(filename, '/')){
         STEPS("Opening ", filename);
         fp = fopen(filename, "r");
     }
     else {
-        ppathNode = (pathNode *) ellFirst(ppathList);
-        while (ppathNode) {
-            fullname = static_cast<char *>(calloc(strlen(filename) + strlen(ppathNode->directory) + 2,
-                sizeof(char)));
-            strcpy(fullname, ppathNode->directory);
-            strcat(fullname, "/");
-            strcat(fullname, filename);
+        pathIt = pathList.begin();
+        while(pathIt != pathList.end()) {
+            fullname = *pathIt + "/" + filename;
             STEPS("Trying", filename);
-            fp = fopen(fullname, "r");
+            fp = fopen(fullname.c_str(), "r");
             if (fp)
                 break;
-            free(fullname);
-            ppathNode = (pathNode *) ellNext(&ppathNode->node);
+            ++pathIt;
         }
     }
 
@@ -562,20 +524,20 @@ static void inputOpenFile(inputData *pinputData, const char * const filename)
     }
 
     STEP("File opened");
-    pinputFile = static_cast<inputFile *>(calloc(1, sizeof(inputFile)));
+    inputFile inFile = inputFile();
 
-    if (ppathNode) {
-        pinputFile->filename = fullname;
+    if (pathIt != pathList.end()) {
+        inFile.filename = fullname;
     }
     else if (filename) {
-        pinputFile->filename = epicsStrDup(filename);
+        inFile.filename = filename;
     }
     else {
-        pinputFile->filename = epicsStrDup("stdin");
+        inFile.filename = "stdin";
     }
 
     if (opt_D) {
-        int hash = epicsStrHash(pinputFile->filename, 12345);
+        int hash = epicsStrHash(inFile.filename.c_str(), 12345);
         int i = 0;
         int match = 0;
 
@@ -588,7 +550,7 @@ static void inputOpenFile(inputData *pinputData, const char * const filename)
         if (!match) {
             const char *wrap = numDeps ? " \\\n" : "";
 
-            printf("%s %s", wrap, pinputFile->filename);
+            printf("%s %s", wrap, inFile.filename.c_str());
             if (numDeps < MAX_DEPS) {
                 depHashes[numDeps++] = hash;
             }
@@ -599,33 +561,29 @@ static void inputOpenFile(inputData *pinputData, const char * const filename)
         }
     }
 
-    pinputFile->fp = fp;
-    ellInsert(&pinputData->inputFileList, 0, &pinputFile->node);
+    inFile.fp = fp;
+    pinputData->inputFileList.push_front(inFile);
     EXIT;
 }
 
 static void inputCloseFile(inputData *pinputData)
 {
-    inputFile *pinputFile;
-
+    std::list<inputFile>& inFileList = pinputData->inputFileList;
     ENTER;
-    pinputFile = (inputFile *) ellFirst(&pinputData->inputFileList);
-    if (pinputFile) {
-        ellDelete(&pinputData->inputFileList, &pinputFile->node);
-        if (fclose(pinputFile->fp))
-            fprintf(stderr, "msi: Can't close input file '%s'\n", pinputFile->filename);
-        free(pinputFile->filename);
-        free(pinputFile);
+    if(!inFileList.empty()) {
+        inputFile& inFile = inFileList.front();
+        if (fclose(inFile.fp))
+            fprintf(stderr, "msi: Can't close input file '%s'\n", inFile.filename.c_str());
+        inFileList.erase(inFileList.begin());
     }
     EXIT;
 }
 
 static void inputCloseAllFiles(inputData *pinputData)
 {
-    inputFile   *pinputFile;
-
     ENTER;
-    while ((pinputFile = (inputFile *) ellFirst(&pinputData->inputFileList))) {
+    const std::list<inputFile>& inFileList = pinputData->inputFileList;
+    while(!inFileList.empty()) {
         inputCloseFile(pinputData);
     }
     EXIT;
@@ -637,7 +595,7 @@ typedef enum {
 } tokenType;
 
 typedef struct subFile {
-    char        *substitutionName;
+    std::string substitutionName;
     FILE        *fp;
     int         lineNum;
     char        inputBuffer[MAX_BUFFER_SIZE];
@@ -646,19 +604,15 @@ typedef struct subFile {
     char        string[MAX_BUFFER_SIZE];
 } subFile;
 
-typedef struct patternNode {
-    ELLNODE     node;
-    char        *var;
-} patternNode;
-
 struct subInfo {
     subFile     *psubFile;
-    int         isFile;
+    bool        isFile;
     char        *filename;
-    int         isPattern;
-    ELLLIST     patternList;
+    bool        isPattern;
+    std::list<std::string> patternList;
     std::string macroReplacements;
-    subInfo() : psubFile(NULL), isFile(0), filename(NULL), isPattern(0) {};
+    subInfo() : psubFile(NULL), isFile(false), filename(NULL),
+                isPattern(false) {};
 };
 
 static char *subGetNextLine(subFile *psubFile);
@@ -677,7 +631,7 @@ void freeSubFile(subInfo *psubInfo)
         if (fclose(psubFile->fp))
             fprintf(stderr, "msi: Can't close substitution file\n");
     }
-    free(psubFile);
+    delete(psubFile);
     free(psubInfo->filename);
     psubInfo->psubFile = 0;
     EXIT;
@@ -685,15 +639,9 @@ void freeSubFile(subInfo *psubInfo)
 
 void freePattern(subInfo *psubInfo)
 {
-    patternNode *ppatternNode;
-
     ENTER;
-    while ((ppatternNode = (patternNode *) ellFirst(&psubInfo->patternList))) {
-        ellDelete(&psubInfo->patternList, &ppatternNode->node);
-        free(ppatternNode->var);
-        free(ppatternNode);
-    }
-    psubInfo->isPattern = 0;
+    psubInfo->patternList.clear();
+    psubInfo->isPattern = false;
     EXIT;
 }
 

@@ -706,7 +654,7 @@ static void substituteDestruct(subInfo * const psubInfo)
     EXIT;
 }
 
-static void substituteOpen(subInfo **ppvt, char * const substitutionName)
+static void substituteOpen(subInfo **ppvt, const std::string& substitutionName)
 {
     subInfo     *psubInfo;
     subFile     *psubFile;
@@ -715,13 +663,12 @@ static void substituteOpen(subInfo **ppvt, char * const substitutionName)
     ENTER;
     psubInfo = new subInfo;
     *ppvt = psubInfo;
-    psubFile = static_cast<subFile *>(calloc(1, sizeof(subFile)));
+    psubFile = new subFile;
     psubInfo->psubFile = psubFile;
-    ellInit(&psubInfo->patternList);
 
-    fp = fopen(substitutionName, "r");
+    fp = fopen(substitutionName.c_str(), "r");
     if (!fp) {
-        fprintf(stderr, "msi: Can't open file '%s'\n", substitutionName);
+        fprintf(stderr, "msi: Can't open file '%s'\n", substitutionName.c_str());
         abortExit(1);
     }
 
@@ -734,7 +681,7 @@ static void substituteOpen(subInfo **ppvt, char * const substitutionName)
     EXIT;
 }
 
-static int substituteGetGlobalSet(subInfo * const psubInfo)
+static bool substituteGetGlobalSet(subInfo * const psubInfo)
 {
     subFile *psubFile = psubInfo->psubFile;
 
@@ -746,17 +693,16 @@ static int substituteGetGlobalSet(subInfo * const psubInfo)
         strcmp(psubFile->string, "global") == 0) {
         subGetNextToken(psubFile);
         EXITD(1);
-        return 1;
+        return true;
     }
 
     EXITD(0);
-    return 0;
+    return false;
 }
 
-static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
+static bool substituteGetNextSet(subInfo * const psubInfo,char **filename)
 {
     subFile     *psubFile = psubInfo->psubFile;
-    patternNode *ppatternNode;
 
     ENTER;
     *filename = 0;
@@ -765,7 +711,7 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
 
     if (psubFile->token == tokenEOF) {
         EXITD(0);
-        return 0;
+        return false;
     }
 
     if (psubFile->token == tokenString &&
@@ -773,7 +719,7 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
         size_t len;
 
         STEP("Parsed 'file'");
-        psubInfo->isFile = 1;
+        psubInfo->isFile = true;
         if (subGetNextToken(psubFile) != tokenString) {
             subFileErrPrint(psubFile, "Parse error, expecting a filename");
             abortExit(1);
@@ -808,7 +754,7 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
 
     if (psubFile->token == tokenLBrace) {
         EXITD(1);
-        return 1;
+        return true;
     }
 
     if (psubFile->token == tokenRBrace) {
@@ -824,7 +770,7 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
 
     STEP("Parsed 'pattern'");
     freePattern(psubInfo);
-    psubInfo->isPattern = 1;
+    psubInfo->isPattern = true;
 
     while (subGetNextToken(psubFile) == tokenSeparator);
 
@@ -834,15 +780,13 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
     }
     STEP("Parsed '{'");
 
-    while (1) {
+    while (true) {
         while (subGetNextToken(psubFile) == tokenSeparator);
 
         if (psubFile->token != tokenString)
             break;
 
-        ppatternNode = static_cast<patternNode *>(calloc(1, sizeof(patternNode)));
-        ellAdd(&psubInfo->patternList, &ppatternNode->node);
-        ppatternNode->var = epicsStrDup(psubFile->string);
+        psubInfo->patternList.push_back(psubFile->string);
     }
 
     if (psubFile->token != tokenRBrace) {
@@ -852,7 +796,7 @@ static int substituteGetNextSet(subInfo * const psubInfo,char **filename)
 
     subGetNextToken(psubFile);
     EXITD(1);
-    return 1;
+    return true;
 }
 

 static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
@@ -866,7 +810,7 @@ static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
         subGetNextToken(psubFile);
 
     if (psubFile->token == tokenRBrace && psubInfo->isFile) {
-        psubInfo->isFile = 0;
+        psubInfo->isFile = false;
         free(psubInfo->filename);
         psubInfo->filename = 0;
         freePattern(psubInfo);
@@ -884,7 +828,7 @@ static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
         return 0;
     }
 
-    while (1) {
+    while (true) {
         switch(subGetNextToken(psubFile)) {
             case tokenRBrace:
                 subGetNextToken(psubFile);
@@ -912,7 +856,6 @@ static const char *substituteGetGlobalReplacements(subInfo * const psubInfo)
 static const char *substituteGetReplacements(subInfo * const psubInfo)
 {
     subFile     *psubFile = psubInfo->psubFile;
-    patternNode *ppatternNode;
 
     ENTER;
     psubInfo->macroReplacements.clear();
@@ -921,7 +864,7 @@ static const char *substituteGetReplacements(subInfo * const psubInfo)
         subGetNextToken(psubFile);
 
     if (psubFile->token==tokenRBrace && psubInfo->isFile) {
-        psubInfo->isFile = 0;
+        psubInfo->isFile = false;
         free(psubInfo->filename);
         psubInfo->filename = 0;
         freePattern(psubInfo);
@@ -941,11 +884,12 @@ static const char *substituteGetReplacements(subInfo * const psubInfo)
     }
 
     if (psubInfo->isPattern) {
-        int gotFirstPattern = 0;
+        bool gotFirstPattern = false;
 
         while (subGetNextToken(psubFile) == tokenSeparator);
-        ppatternNode = (patternNode *) ellFirst(&psubInfo->patternList);
-        while (1) {
+        std::list<std::string>& patternList = psubInfo->patternList;
+        std::list<std::string>::iterator patternIt = patternList.begin();
+        while (true) {
             if (psubFile->token == tokenRBrace) {
                 subGetNextToken(psubFile);
                 EXITS(psubInfo->macroReplacements.c_str());
@@ -959,13 +903,13 @@ static const char *substituteGetReplacements(subInfo * const psubInfo)
 
             if (gotFirstPattern)
                 catMacroReplacements(psubInfo, ",");
-            gotFirstPattern = 1;
+            gotFirstPattern = true;
 
-            if (ppatternNode) {
-                catMacroReplacements(psubInfo, ppatternNode->var);
+            if (patternIt != patternList.end()) {
+                catMacroReplacements(psubInfo, patternIt->c_str());
                 catMacroReplacements(psubInfo, "=");
                 catMacroReplacements(psubInfo, psubFile->string);
-                ppatternNode = (patternNode *) ellNext(&ppatternNode->node);
+                ++patternIt;
             }
             else {
                 subFileErrPrint(psubFile, "Warning, too many values given");
@@ -974,7 +918,7 @@ static const char *substituteGetReplacements(subInfo * const psubInfo)
             while (subGetNextToken(psubFile) == tokenSeparator);
         }
     }
-    else while(1) {
+    else while(true) {
         switch(subGetNextToken(psubFile)) {
             case tokenRBrace:
                 subGetNextToken(psubFile);
@@ -1026,7 +970,8 @@ static void subFileErrPrint(subFile *psubFile, const char * message)
 {
     fprintf(stderr, "msi: %s\n",message);
     fprintf(stderr, "  in substitution file '%s' at line %d:\n  %s",
-        psubFile->substitutionName, psubFile->lineNum, psubFile->inputBuffer);
+            psubFile->substitutionName.c_str(), psubFile->lineNum,
+            psubFile->inputBuffer);
 }
 
 

diff --git a/src/ioc/dbtemplate/test/msi.plt b/src/ioc/dbtemplate/test/msi.plt
index 332defb..ca87ca1 100644
--- a/src/ioc/dbtemplate/test/msi.plt
+++ b/src/ioc/dbtemplate/test/msi.plt
@@ -11,7 +11,7 @@
 use strict;
 use Test;
 
-BEGIN {plan tests => 11}
+BEGIN {plan tests => 12}
 
 # Check include/substitute command model
 ok(msi('-I .. ../t1-template.txt'),             slurp('../t1-result.txt'));
@@ -56,6 +56,8 @@ ok(msi('-I. -I.. -S ../t10-substitute.txt'), slurp('../t10-result.txt'));
 # Substitution file, pattern format, with 0 pattern definitions
 ok(msi('-I. -I.. -S ../t11-substitute.txt'), slurp('../t11-result.txt'));
 
+# Macros in template-file name populated from environment variable
+ok(msi('-I. -I.. -S ../t12-substitute.txt', 'TEST_NO=12,PREFIX=t'), slurp('../t12-result.txt'));
 
 # Test support routines
 
@@ -68,10 +70,16 @@ sub slurp {
 }
 
 sub msi {
-    my ($args) = @_;
+    my ($args, $envstr) = @_;
     my $exe = ($^O eq 'MSWin32') || ($^O eq 'cygwin') ? '.exe' : '';
     my $msi = "./msi-copy$exe";
     my $result;
+    my @envs = split(/,/, $envstr);
+    foreach (@envs)
+    {
+        my ($var, $value) = split /=/, $_;
+        $ENV{$var} = $value;
+    }
     if ($args =~ m/-o / && $args !~ m/-D/) {
         # An empty result is expected
         $result = `$msi $args`;
@@ -85,5 +93,10 @@ sub msi {
                 if $result eq '';
         } while ($result eq '') && (--$count > 0);
     }
+    foreach (@envs)
+    {
+        my ($var, $value) = split /=/, $_;
+        delete $ENV{$var};
+    }
     return $result;
 }
diff --git a/src/ioc/dbtemplate/test/t12-result.txt b/src/ioc/dbtemplate/test/t12-result.txt
new file mode 100644
index 0000000..6cfcc57
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t12-result.txt
@@ -0,0 +1,2 @@
+# comment line
+a=foo
diff --git a/src/ioc/dbtemplate/test/t12-substitute.txt b/src/ioc/dbtemplate/test/t12-substitute.txt
new file mode 100644
index 0000000..7a26b6f
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t12-substitute.txt
@@ -0,0 +1,3 @@
+file $(PREFIX)$(TEST_NO)-template.txt {
+    { a=foo }
+}
diff --git a/src/ioc/dbtemplate/test/t12-template.txt b/src/ioc/dbtemplate/test/t12-template.txt
new file mode 100644
index 0000000..7958885
--- /dev/null
+++ b/src/ioc/dbtemplate/test/t12-template.txt
@@ -0,0 +1,2 @@
+# comment line
+a=$(a)