Merge pull request #2727 from akohlmey/refactor-read-lines-from-file

Move "read_lines_from_file" utility function from Comm class to utils namespace

doc/src/Developer_utils.rst

@@ -12,11 +12,18 @@ reduces redundant implementations and encourages consistent behavior.
 I/O with status check
 ^^^^^^^^^^^^^^^^^^^^^
 
-These are wrappers around the corresponding C library calls like
+The the first two functions are wrappers around the corresponding C
-``fgets()`` or ``fread()``.  They will check if there were errors
+library calls ``fgets()`` or ``fread()``.  They will check if there
-on reading or an unexpected end-of-file state was reached.  In that
+were errors on reading or an unexpected end-of-file state was reached.
-case, the functions will stop the calculation with an error message,
+In that case, the functions will stop with an error message, indicating
-indicating the name of the problematic file, if possible.
+the name of the problematic file, if possible unless the *error* argument
+is a NULL pointer.
+
+The :cpp:func:`read_lines_from_file` function will read the requested
+number of lines of a maximum length into a buffer and will return 0
+if successful or 1 if not. It also guarantees that all lines are
+terminated with a newline character and the entire buffer with a
+NULL character.
 
 ----------
 
@@ -26,6 +33,9 @@ indicating the name of the problematic file, if possible.
 .. doxygenfunction:: sfread
    :project: progguide
 
+.. doxygenfunction:: read_lines_from_file
+   :project: progguide
+
 ----------
 
 String to number conversions with validity check

doc/utils/sphinx-config/false_positives.txt

@@ -2190,6 +2190,7 @@ nl
 nlayers
 nlen
 Nlines
+nlines
 nlo
 nlocal
 Nlocal
@@ -2281,6 +2282,7 @@ Ntype
 ntypes
 Ntypes
 nucleotides
+nullptr
 num
 numa
 numactl

lib/atc/LammpsInterface.cpp

@@ -26,6 +26,7 @@
 #include "bond.h" // bond potentials
 #include "comm.h" //
 #include "fix.h"
+#include "utils.h"
 
 // ATC includes
 #include "ATC_Error.h"
@@ -47,6 +48,7 @@ using std::pair;
 using std::string;
 using std::set;
 using LAMMPS_NS::bigint;
+using LAMMPS_NS::utils::read_lines_from_file;
 
 namespace ATC {
 
@@ -236,7 +238,7 @@ std::string LammpsInterface::read_file(std::string filename) const
   std::stringstream s;
   bool eof = false;
   while ( ! eof) {
-    eof = lammps_->comm->read_lines_from_file(fp,1,MAXLINE,buffer);
+    eof = read_lines_from_file(fp,1,MAXLINE,buffer,comm_rank(),lammps_->world);
     s << buffer;
   }
   fclose(fp);

src/REPLICA/neb.cpp

@@ -436,9 +436,10 @@ void NEB::readfile(char *file, int flag)
   while (nread < nlines) {
     nchunk = MIN(nlines-nread,CHUNK);
     if (flag == 0)
-      eofflag = comm->read_lines_from_file_universe(fp,nchunk,MAXLINE,buffer);
+      eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,
+                                            universe->me,universe->uworld);
     else
-      eofflag = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+      eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eofflag) error->all(FLERR,"Unexpected end of NEB file");
 
     buf = buffer;

src/RIGID/fix_rigid.cpp

@@ -2302,7 +2302,7 @@ void FixRigid::readfile(int which, double *vec,
   int nread = 0;
   while (nread < nlines) {
     nchunk = MIN(nlines-nread,CHUNK);
-    eofflag = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eofflag) error->all(FLERR,"Unexpected end of fix rigid file");
 
     buf = buffer;

src/RIGID/fix_rigid_small.cpp

@@ -2475,7 +2475,7 @@ void FixRigidSmall::readfile(int which, double **array, int *inbody)
   int nread = 0;
   while (nread < nlines) {
     nchunk = MIN(nlines-nread,CHUNK);
-    eofflag = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eofflag) error->all(FLERR,"Unexpected end of fix rigid/small file");
 
     buf = buffer;

src/SPIN/neb_spin.cpp

@@ -430,9 +430,10 @@ void NEBSpin::readfile(char *file, int flag)
   while (nread < nlines) {
     nchunk = MIN(nlines-nread,CHUNK);
     if (flag == 0)
-      eofflag = comm->read_lines_from_file_universe(fp,nchunk,MAXLINE,buffer);
+      eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,
+                                            universe->me,universe->uworld);
     else
-      eofflag = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+      eofflag = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eofflag) error->all(FLERR,"Unexpected end of neb/spin file");
 
     buf = buffer;

src/comm.cpp

@@ -1240,71 +1240,3 @@ void Comm::rendezvous_stats(int n, int nout, int nrvous, int nrvous_out,
     utils::logmesg(lmp,mesg);
   }
 }
-
-/* ----------------------------------------------------------------------
-   proc 0 reads Nlines from file into buf and bcasts buf to all procs
-   caller allocates buf to max size needed
-   each line is terminated by newline, even if last line in file is not
-   return 0 if successful, 1 if get EOF error before read is complete
-------------------------------------------------------------------------- */
-
-int Comm::read_lines_from_file(FILE *fp, int nlines, int maxline, char *buf)
-{
-  int m;
-
-  if (me == 0) {
-    m = 0;
-    for (int i = 0; i < nlines; i++) {
-      if (!fgets(&buf[m],maxline,fp)) {
-        m = 0;
-        break;
-      }
-      m += strlen(&buf[m]);
-    }
-    if (m) {
-      if (buf[m-1] != '\n') strcpy(&buf[m++],"\n");
-      m++;
-    }
-  }
-
-  MPI_Bcast(&m,1,MPI_INT,0,world);
-  if (m == 0) return 1;
-  MPI_Bcast(buf,m,MPI_CHAR,0,world);
-  return 0;
-}
-
-/* ----------------------------------------------------------------------
-   proc 0 reads Nlines from file into buf and bcasts buf to all procs
-   caller allocates buf to max size needed
-   each line is terminated by newline, even if last line in file is not
-   return 0 if successful, 1 if get EOF error before read is complete
-------------------------------------------------------------------------- */
-
-int Comm::read_lines_from_file_universe(FILE *fp, int nlines, int maxline,
-                                        char *buf)
-{
-  int m;
-
-  int me_universe = universe->me;
-  MPI_Comm uworld = universe->uworld;
-
-  if (me_universe == 0) {
-    m = 0;
-    for (int i = 0; i < nlines; i++) {
-      if (!fgets(&buf[m],maxline,fp)) {
-        m = 0;
-        break;
-      }
-      m += strlen(&buf[m]);
-    }
-    if (m) {
-      if (buf[m-1] != '\n') strcpy(&buf[m++],"\n");
-      m++;
-    }
-  }
-
-  MPI_Bcast(&m,1,MPI_INT,0,uworld);
-  if (m == 0) return 1;
-  MPI_Bcast(buf,m,MPI_CHAR,0,uworld);
-  return 0;
-}

src/comm.h

@@ -114,9 +114,6 @@ class Comm : protected Pointers {
                  int (*)(int, char *, int &, int *&, char *&, void *),
                  int, char *&, int, void *, int statflag=0);
 
-  int read_lines_from_file(FILE *, int, int, char *);
-  int read_lines_from_file_universe(FILE *, int, int, char *);
-
   // extract data useful to other classes
   virtual void *extract(const char *, int &) {return nullptr;}
 

src/read_data.cpp

@@ -1226,7 +1226,7 @@ void ReadData::atoms()
 
   while (nread < natoms) {
     nchunk = MIN(natoms-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_atoms(nchunk,buffer,id_offset,mol_offset,toffset,shiftflag,shift);
     nread += nchunk;
@@ -1282,7 +1282,7 @@ void ReadData::velocities()
 
   while (nread < natoms) {
     nchunk = MIN(natoms-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_vels(nchunk,buffer,id_offset);
     nread += nchunk;
@@ -1324,7 +1324,7 @@ void ReadData::bonds(int firstpass)
 
   while (nread < nbonds) {
     nchunk = MIN(nbonds-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_bonds(nchunk,buffer,count,id_offset,boffset);
     nread += nchunk;
@@ -1398,7 +1398,7 @@ void ReadData::angles(int firstpass)
 
   while (nread < nangles) {
     nchunk = MIN(nangles-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_angles(nchunk,buffer,count,id_offset,aoffset);
     nread += nchunk;
@@ -1472,7 +1472,7 @@ void ReadData::dihedrals(int firstpass)
 
   while (nread < ndihedrals) {
     nchunk = MIN(ndihedrals-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_dihedrals(nchunk,buffer,count,id_offset,doffset);
     nread += nchunk;
@@ -1546,7 +1546,7 @@ void ReadData::impropers(int firstpass)
 
   while (nread < nimpropers) {
     nchunk = MIN(nimpropers-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_impropers(nchunk,buffer,count,id_offset,ioffset);
     nread += nchunk;
@@ -1613,7 +1613,7 @@ void ReadData::bonus(bigint nbonus, AtomVec *ptr, const char *type)
 
   while (nread < natoms) {
     nchunk = MIN(natoms-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     atom->data_bonus(nchunk,buffer,ptr,id_offset);
     nread += nchunk;
@@ -1739,7 +1739,7 @@ void ReadData::mass()
   char *next;
   char *buf = new char[ntypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,ntypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,ntypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1759,7 +1759,7 @@ void ReadData::paircoeffs()
   char *next;
   char *buf = new char[ntypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,ntypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,ntypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1785,7 +1785,7 @@ void ReadData::pairIJcoeffs()
   int nsq = ntypes * (ntypes+1) / 2;
   char *buf = new char[nsq * MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,nsq,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,nsq,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1811,7 +1811,7 @@ void ReadData::bondcoeffs()
   char *next;
   char *buf = new char[nbondtypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,nbondtypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,nbondtypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1836,7 +1836,7 @@ void ReadData::anglecoeffs(int which)
   char *next;
   char *buf = new char[nangletypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,nangletypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,nangletypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1862,7 +1862,7 @@ void ReadData::dihedralcoeffs(int which)
   char *next;
   char *buf = new char[ndihedraltypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,ndihedraltypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,ndihedraltypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1892,7 +1892,7 @@ void ReadData::impropercoeffs(int which)
   char *next;
   char *buf = new char[nimpropertypes*MAXLINE];
 
-  int eof = comm->read_lines_from_file(fp,nimpropertypes,MAXLINE,buf);
+  int eof = utils::read_lines_from_file(fp,nimpropertypes,MAXLINE,buf,me,world);
   if (eof) error->all(FLERR,"Unexpected end of data file");
 
   char *original = buf;
@@ -1922,7 +1922,7 @@ void ReadData::fix(int ifix, char *keyword)
   bigint nread = 0;
   while (nread < nline) {
     nchunk = MIN(nline-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) error->all(FLERR,"Unexpected end of data file");
     modify->fix[ifix]->read_data_section(keyword,nchunk,buffer,id_offset);
     nread += nchunk;

src/utils.cpp

@@ -225,6 +225,36 @@ void utils::sfread(const char *srcname, int srcline, void *s, size_t size,
 
 /* ------------------------------------------------------------------ */
 
+/* read N lines and broadcast */
+int utils::read_lines_from_file(FILE *fp, int nlines, int nmax,
+                                char *buffer, int me, MPI_Comm comm)
+{
+  char *ptr = buffer;
+  *ptr = '\0';
+
+  if (me == 0) {
+    if (fp) {
+      for (int i = 0; i < nlines; i++) {
+        ptr = fgets(ptr,nmax,fp);
+        if (!ptr) break; // EOF?
+        // advance ptr to end of string and append newline char if needed.
+        ptr += strlen(ptr);
+        if (*(--ptr) != '\n') *(++ptr) = '\n';
+        // ensure buffer is null terminated. null char is start of next line.
+        *(++ptr) = '\0';
+      }
+    }
+  }
+
+  int n = strlen(buffer);
+  MPI_Bcast(&n,1,MPI_INT,0,comm);
+  if (n == 0) return 1;
+  MPI_Bcast(buffer,n+1,MPI_CHAR,0,comm);
+  return 0;
+}
+
+/* ------------------------------------------------------------------ */
+
 std::string utils::check_packages_for_style(const std::string &style,
                                             const std::string &name,
                                             LAMMPS *lmp)

src/utils.h

@@ -17,9 +17,12 @@
 /*! \file utils.h */
 
 #include "lmptype.h"
+
+#include <mpi.h>
+
+#include <cstdio>
 #include <string>
 #include <vector>
-#include <cstdio>
 
 namespace LAMMPS_NS {
 
@@ -52,7 +55,7 @@ namespace LAMMPS_NS {
 
     void logmesg(LAMMPS *lmp, const std::string &mesg);
 
-    /** return a string representing the current system error status
+    /** Return a string representing the current system error status
      *
      *  This is a wrapper around calling strerror(errno).
      *
@@ -60,8 +63,10 @@ namespace LAMMPS_NS {
 
     std::string getsyserror();
 
-    /** safe wrapper around fgets() which aborts on errors
+    /** Safe wrapper around fgets() which aborts on errors
-     *  or EOF and prints a suitable error message to help debugging
+     *  or EOF and prints a suitable error message to help debugging.
+     *
+     *  Use nullptr as the error parameter to avoid the abort on EOF or error.
      *
      *  \param srcname  name of the calling source file (from FLERR macro)
      *  \param srcline  line in the calling source file (from FLERR macro)
@@ -69,13 +74,15 @@ namespace LAMMPS_NS {
      *  \param size     size of buffer s (max number of bytes read by fgets())
      *  \param fp       file pointer used by fgets()
      *  \param filename file name associated with fp (may be a null pointer; then LAMMPS will try to detect)
-     *  \param error    pointer to Error class instance (for abort) */
+     *  \param error    pointer to Error class instance (for abort) or nullptr */
 
     void sfgets(const char *srcname, int srcline, char *s, int size,
                 FILE *fp, const char *filename, Error *error);
 
-    /** safe wrapper around fread() which aborts on errors
+    /** Safe wrapper around fread() which aborts on errors
-     *  or EOF and prints a suitable error message to help debugging
+     *  or EOF and prints a suitable error message to help debugging.
+     *
+     *  Use nullptr as the error parameter to avoid the abort on EOF or error.
      *
      *  \param srcname  name of the calling source file (from FLERR macro)
      *  \param srcline  line in the calling source file (from FLERR macro)
@@ -84,11 +91,31 @@ namespace LAMMPS_NS {
      *  \param num      number of data elements read by fread()
      *  \param fp       file pointer used by fread()
      *  \param filename file name associated with fp (may be a null pointer; then LAMMPS will try to detect)
-     *  \param error    pointer to Error class instance (for abort) */
+     *  \param error    pointer to Error class instance (for abort) or nullptr */
 
     void sfread(const char *srcname, int srcline, void *s, size_t size,
                 size_t num, FILE *fp, const char *filename, Error *error);
 
+    /** Read N lines of text from file into buffer and broadcast them
+     *
+     * This function uses repeated calls to fread() to fill a buffer with
+     * newline terminated text.  If a line does not end in a newline (e.g.
+     * at the end of a file), it is added.  The caller has to allocate an
+     * nlines by nmax sized buffer for storing the text data.
+     * Reading is done by MPI rank 0 of the given communicator only, and
+     * thus only MPI rank 0 needs to provide a valid file pointer.
+     *
+     *  \param fp       file pointer used by fread
+     *  \param nlines   number of lines to be read
+     *  \param nmax     maximum length of a single line
+     *  \param buffer   buffer for storing the data.
+     *  \param me       MPI rank of calling process in MPI communicator
+     *  \param comm     MPI communicator for broadcast
+     *  \return         1 if the read was short, 0 if read was successful */
+
+    int read_lines_from_file(FILE *fp, int nlines, int nmax,
+                             char *buffer, int me, MPI_Comm comm);
+
     /** Report if a requested style is in a package or may have a typo
      *
      *  \param style type of style that is to be checked for

src/variable.cpp

@@ -5176,7 +5176,7 @@ int VarReader::read_peratom()
   bigint nread = 0;
   while (nread < nlines) {
     nchunk = MIN(nlines-nread,CHUNK);
-    eof = comm->read_lines_from_file(fp,nchunk,MAXLINE,buffer);
+    eof = utils::read_lines_from_file(fp,nchunk,MAXLINE,buffer,me,world);
     if (eof) return 1;
 
     char *buf = buffer;

unittest/formats/test_file_operations.cpp

@@ -11,13 +11,13 @@
    See the README file in the top-level LAMMPS directory.
 ------------------------------------------------------------------------- */
 
+#include "../testing/core.h"
 #include "info.h"
 #include "input.h"
 #include "lammps.h"
 #include "utils.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
-#include "../testing/core.h"
 
 #include <cstdio>
 #include <mpi.h>
@@ -28,6 +28,7 @@ using namespace LAMMPS_NS;
 using testing::MatchesRegex;
 using testing::StrEq;
 
+using utils::read_lines_from_file;
 using utils::sfgets;
 using utils::sfread;
 using utils::split_words;
@@ -124,6 +125,37 @@ TEST_F(FileOperationsTest, safe_fread)
     fclose(fp);
 }
 
+TEST_F(FileOperationsTest, read_lines_from_file)
+{
+    char *buf = new char[MAX_BUF_SIZE];
+    FILE *fp       = nullptr;
+    MPI_Comm world = MPI_COMM_WORLD;
+    int me, rv;
+    memset(buf, 0, MAX_BUF_SIZE);
+
+    rv = utils::read_lines_from_file(nullptr, 1, MAX_BUF_SIZE, buf, me, world);
+    ASSERT_EQ(rv, 1);
+
+    MPI_Comm_rank(world, &me);
+    if (me == 0) {
+        fp = fopen("safe_file_read_test.txt", "r");
+        ASSERT_NE(fp, nullptr);
+    } else
+        ASSERT_EQ(fp, nullptr);
+
+    rv = utils::read_lines_from_file(fp, 2, MAX_BUF_SIZE / 2, buf, me, world);
+    ASSERT_EQ(rv, 0);
+    ASSERT_THAT(buf, StrEq("one line\ntwo_lines\n"));
+
+    rv = utils::read_lines_from_file(fp, 2, MAX_BUF_SIZE / 2, buf, me, world);
+    ASSERT_EQ(rv, 0);
+    ASSERT_THAT(buf, StrEq("\nno newline\n"));
+
+    rv = utils::read_lines_from_file(fp, 2, MAX_BUF_SIZE / 2, buf, me, world);
+    ASSERT_EQ(rv, 1);
+    delete[] buf;
+}
+
 TEST_F(FileOperationsTest, logmesg)
 {
     char buf[8];