replace std::ostream Topo::warnings with a Logger class

Logger has a user-defined callback

experimental

perhaps will be used also in other functions that give warnings or messages

include/gemmi/topo.hpp

@@ -5,8 +5,8 @@
 #ifndef GEMMI_TOPO_HPP_
 #define GEMMI_TOPO_HPP_
 
+#include <functional>    // for function
 #include <map>           // for multimap
-#include <ostream>       // for ostream
 #include <memory>        // for unique_ptr
 #include <unordered_map> // for unordered_map
 #include "chemcomp.hpp"  // for ChemComp
@@ -20,6 +20,35 @@ enum class HydrogenChange {
   NoChange, Shift, Remove, ReAdd, ReAddButWater, ReAddKnown
 };
 
+
+struct Logger {
+  using Callback = std::function<void(const std::string&)>;
+  Callback callback;
+
+  // For internal use in functions that produce messages: suspending when
+  // the same function is called multiple times avoids duplicated messages.
+  bool suspended = false;
+
+  template<class... Args> GEMMI_COLD void err(Args const&... args) const {
+    if (!suspended) {
+      std::string msg = cat(args...);
+      if (callback == nullptr)
+        fail(msg);
+      callback("Warning: " + msg);
+    }
+  }
+
+  template<class... Args> void mesg(Args const&... args) const {
+    if (!suspended && callback)
+      callback(cat(args...));
+  }
+
+  static void to_stderr(const std::string& s) {
+    std::fprintf(stderr, "%s\n", s.c_str());
+  }
+};
+
+
 struct GEMMI_DLL Topo {
   // We have internal pointers in this class (pointers setup in
   // apply_restraints() that point to ResInfo::chemcomp.rt),
@@ -184,7 +213,7 @@ struct GEMMI_DLL Topo {
     return -1;
   }
 
-  std::ostream* warnings = nullptr;
+  Logger logger{};
   bool only_bonds = false;  // an internal flag for apply_restraints()
   std::vector<ChainInfo> chain_infos;
   std::vector<Link> extras;
@@ -274,12 +303,6 @@ struct GEMMI_DLL Topo {
 
   void set_cispeps_in_structure(Structure& st);
 
-  GEMMI_COLD void err(const std::string& msg) const {
-    if (warnings == nullptr)
-      fail(msg);
-    *warnings << "Warning: " << msg << std::endl;
-  }
-
 private:
   // storage for link restraints modified by aliases
   std::vector<std::unique_ptr<Restraints>> rt_storage;
@@ -295,7 +318,7 @@ struct GEMMI_DLL Topo {
 GEMMI_DLL std::unique_ptr<Topo>
 prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
                  HydrogenChange h_change, bool reorder,
-                 std::ostream* warnings=nullptr, bool ignore_unknown_links=false,
+                 const Logger::Callback& callback={}, bool ignore_unknown_links=false,
                  bool use_cispeps=false);
 
 

prog/crd.cpp

@@ -1,7 +1,7 @@
 // Copyright 2017-2022 Global Phasing Ltd.
 
 #include <stdio.h>             // for fprintf, stderr, putc
-#include <iostream>            // for cerr
+#include <iostream>            // for cout
 #include <exception>           // for exception
 #include "gemmi/crd.hpp"       // for prepare_refmac_crd
 #include "gemmi/to_cif.hpp"    // for write_cif_block_to_stream
@@ -122,7 +122,7 @@ int GEMMI_MAIN(int argc, char **argv) {
     else
       h_change = HydrogenChange::ReAddButWater;
     auto topo = prepare_topology(st, monlib, 0, h_change, reorder,
-                                 &std::cerr, ignore_unknown_links, use_cispeps);
+                                 &Logger::to_stderr, ignore_unknown_links, use_cispeps);
     if (!use_cispeps)
       topo->set_cispeps_in_structure(st);
     if (verbose)

prog/h.cpp

@@ -132,7 +132,7 @@ int GEMMI_MAIN(int argc, char **argv) {
       }
       for (size_t i = 0; i != st.models.size(); ++i) {
         // preparing topology modifies hydrogens in the model
-        prepare_topology(st, monlib, i, h_change, p.options[Sort], &std::cerr);
+        prepare_topology(st, monlib, i, h_change, p.options[Sort], &gemmi::Logger::to_stderr);
       }
     }
     if (p.options[Verbose])

prog/rmsz.cpp

@@ -2,7 +2,6 @@
 
 #include <stdio.h>   // for printf, fprintf, stderr
 #include <cmath>     // for sqrt
-#include <iostream>  // for cerr
 #include <exception>
 #include "gemmi/model.hpp"     // for Structure, Atom, etc
 #include "gemmi/chemcomp.hpp"  // for ChemComp
@@ -239,7 +238,7 @@ int GEMMI_MAIN(int argc, char **argv) {
         if (st.models.size() > 1)
           printf("### Model %s ###\n", model.name.c_str());
         Topo topo;
-        topo.warnings = &std::cerr;
+        topo.logger.callback = &gemmi::Logger::to_stderr;
         topo.initialize_refmac_topology(st, model, monlib);
         topo.apply_all_restraints(monlib);
 

python/topo.cpp

@@ -25,6 +25,18 @@ NB_MAKE_OPAQUE(std::vector<Topo::Rule>)
 NB_MAKE_OPAQUE(std::vector<Topo::Mod>)
 NB_MAKE_OPAQUE(std::vector<Topo::FinalChemComp>)
 
+gemmi::Logger::Callback as_callback(const nb::object& warnings) {
+  if (warnings.is_none())
+    return {};
+  if (nb::hasattr(warnings, "write") && nb::hasattr(warnings, "flush"))
+    return [&](const std::string& s) {
+      warnings.attr("write")(nb::str((s + "\n").c_str()));
+      warnings.attr("flush")();
+    };
+  return [&](const std::string& s) {
+      warnings(nb::str(s.c_str()));
+  };
+}
 
 void add_topo(nb::module_& m) {
   nb::class_<Topo> topo(m, "Topo");
@@ -166,18 +178,9 @@ void add_topo(nb::module_& m) {
   m.def("prepare_topology",
     [](Structure& st, MonLib& monlib, size_t model_index,
        HydrogenChange h_change, bool reorder,
-       const nb::object& pywarnings, bool ignore_unknown_links, bool use_cispeps) {
-      std::ostream* warnings = nullptr;
-      // TODO: new way of passing warnings
-      //std::ostream os(nullptr);
-      //std::unique_ptr<nb::detail::pythonbuf> buffer;
-      //if (!pywarnings.is_none()) {
-      //  buffer.reset(new nb::detail::pythonbuf(pywarnings));
-      //  os.rdbuf(buffer.get());
-      //  warnings = &os;
-      //}
+       const nb::object& warnings, bool ignore_unknown_links, bool use_cispeps) {
       return prepare_topology(st, monlib, model_index, h_change, reorder,
-                              warnings, ignore_unknown_links, use_cispeps);
+                              as_callback(warnings), ignore_unknown_links, use_cispeps);
     }, nb::arg("st"), nb::arg("monlib"), nb::arg("model_index")=0,
        nb::arg("h_change")=HydrogenChange::NoChange, nb::arg("reorder")=false,
        nb::arg("warnings")=nb::none(), nb::arg("ignore_unknown_links")=false,

src/riding_h.cpp

@@ -287,8 +287,7 @@ void place_hydrogens(const Topo& topo, const Atom& atom,
       Vec3 v14 = rotate_about_axis(v12, axis, theta1 * ratio);
       hs[0].pos = atom.pos + Position(hs[0].dist / v14.length() * v14);
       if (hs.size() > 1) {
-        topo.err("Unhandled topology of " + std::to_string(hs.size()) +
-                 " hydrogens bonded to " + atom.name);
+        topo.logger.err("Unhandled topology of ", hs.size(), " hydrogens bonded to ", atom.name);
         for (size_t i = 1; i < hs.size(); ++i) {
           hs[i].ptr->occ = 0;
           hs[i].ptr->calc_flag = CalcFlag::Dummy;
@@ -424,9 +423,9 @@ void place_hydrogens_on_all_atoms(Topo& topo) {
           if (altlocs.empty())
             place_hydrogens(topo, atom, known, hs);
         } catch (const std::runtime_error& e) {
-          topo.err("Placing of hydrogen bonded to "
-                   + atom_str(chain_info.chain_ref, *ri.res, atom)
-                   + " failed:\n  " + e.what());
+          topo.logger.err("Placing of hydrogen bonded to ",
+                          atom_str(chain_info.chain_ref, *ri.res, atom),
+                          " failed:\n  ", e.what());
         }
       }
     }

src/topo.cpp

@@ -428,13 +428,13 @@ void Topo::apply_restraints_from_link(Link& link, const MonLib& monlib) {
     return;
   const ChemLink* chem_link = monlib.get_link(link.link_id);
   if (!chem_link) {
-    err("ignoring link '" + link.link_id + "' as it is not in the monomer library");
+    logger.err("ignoring link '", link.link_id, "' as it is not in the monomer library");
     return;
   }
   const Restraints* rt = &chem_link->rt;
   if (link.alt1 && link.alt2 && link.alt1 != link.alt2)
-    err(cat("LINK between different conformers: ", link.alt1, " (in ",
-            link.res1->name, ") and ", link.alt2, " (in " + link.res2->name, ")."));
+    logger.err("LINK between different conformers: ", link.alt1, " (in ",
+               link.res1->name, ") and ", link.alt2, " (in ", link.res2->name, ").");
   // aliases are a new feature - introduced in 2022
   if (link.aliasing1 || link.aliasing2) {
     std::unique_ptr<Restraints> rt_copy(new Restraints(*rt));
@@ -559,11 +559,11 @@ void Topo::initialize_refmac_topology(Structure& st, Model& model0,
                   try {
                     chem_mod->apply_to(*cc_copy, mod.alias);
                   } catch(std::runtime_error& e) {
-                    err(cat("failed to apply modification ", chem_mod->id,
-                            " to ", ri.res->name, ": ", e.what()));
+                    logger.err("failed to apply modification ", chem_mod->id,
+                               " to ", ri.res->name, ": ", e.what());
                   }
                 } else {
-                  err("modification not found: " + mod.id);
+                  logger.err("modification not found: ", mod.id);
                 }
               }
             }
@@ -687,7 +687,7 @@ void Topo::setup_connection(Connection& conn, Model& model0, MonLib& monlib,
   if (!conn.link_id.empty()) {
     match = monlib.get_link(conn.link_id);
     if (!match) {
-      err("link not found in monomer library: " + conn.link_id);
+      logger.err("link not found in monomer library: ", conn.link_id);
       return;
     }
     if (match->rt.bonds.empty() ||
@@ -697,7 +697,7 @@ void Topo::setup_connection(Connection& conn, Model& model0, MonLib& monlib,
                                conn.partner1.atom_name, extra.aliasing1) ||
         !atom_match_with_alias(match->rt.bonds[0].id2.atom,
                                conn.partner2.atom_name, extra.aliasing2)) {
-      err("link from the monomer library does not match: " + conn.link_id);
+      logger.err("link from the monomer library does not match: ", conn.link_id);
       return;
     }
   } else {
@@ -801,8 +801,7 @@ void set_cis_in_link(Topo::Link& link, bool is_cis) {
 }
 
 void force_cispeps(Topo& topo, bool single_model, const Model& model,
-                          const std::vector<CisPep>& cispeps,
-                          std::ostream* warnings) {
+                          const std::vector<CisPep>& cispeps) {
   std::multimap<const Residue*, const CisPep*> cispep_index;
   for (const CisPep& cp : cispeps) {
     if (single_model || model.name == cp.model_str)
@@ -823,12 +822,10 @@ void force_cispeps(Topo& topo, bool single_model, const Model& model,
         }
         if (is_cis != link.is_cis) {
           set_cis_in_link(link, is_cis);
-          if (warnings)
-            *warnings << "Link between "
-                      << atom_str(chain_info.chain_ref.name, *link.res1, "", link.alt1)
-                      << " and "
-                      << atom_str(chain_info.chain_ref.name, *link.res2, "", link.alt2)
-                      << " forced to " << link.link_id << std::endl;
+          const std::string& ch_name = chain_info.chain_ref.name;
+          topo.logger.mesg("Link between ", atom_str(ch_name, *link.res1, "", link.alt1),
+                           " and ", atom_str(ch_name, *link.res2, "", link.alt2),
+                           " forced to ", link.link_id);
         }
       }
     }
@@ -902,17 +899,13 @@ void add_hydrogens_without_positions(Topo::ResInfo& ri, const NeighMap& neighbor
 
 NeighMap prepare_neighbor_altlocs(Topo& topo, const MonLib& monlib) {
   // disable warnings here, so they are not printed twice
-  std::streambuf* warnings_orig = nullptr;
-  if (topo.warnings)
-    warnings_orig = topo.warnings->rdbuf(nullptr);
+  topo.logger.suspended = true;
   // Prepare bonds. Fills topo.bonds, monomer_rules/link_rules and rt_storage,
   // but they are all reset when apply_all_restraints() is called again.
   topo.only_bonds = true;
   topo.apply_all_restraints(monlib);
   topo.only_bonds = false;
-  // re-enable warnings
-  if (warnings_orig)
-    topo.warnings->rdbuf(warnings_orig);
+  topo.logger.suspended = false; // re-enable warnings
   NeighMap neighbors;
   for (const Topo::Bond& bond : topo.bonds) {
     const Atom* a1 = bond.atoms[0];
@@ -930,16 +923,16 @@ NeighMap prepare_neighbor_altlocs(Topo& topo, const MonLib& monlib) {
 std::unique_ptr<Topo>
 prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
                  HydrogenChange h_change, bool reorder,
-                 std::ostream* warnings, bool ignore_unknown_links, bool use_cispeps) {
+                 const Logger::Callback& callback, bool ignore_unknown_links, bool use_cispeps) {
   std::unique_ptr<Topo> topo(new Topo);
-  topo->warnings = warnings;
+  topo->logger.callback = callback;
   if (model_index >= st.models.size())
     fail("no such model index: " + std::to_string(model_index));
   Model& model = st.models[model_index];
   topo->initialize_refmac_topology(st, model, monlib, ignore_unknown_links);
 
   if (use_cispeps)
-    force_cispeps(*topo, st.models.size() == 1, model, st.cispeps, warnings);
+    force_cispeps(*topo, st.models.size() == 1, model, st.cispeps);
 
   // remove hydrogens, or change deuterium to fraction, or nothing
   // and then check atom names
@@ -978,7 +971,7 @@ prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
             if (it != cc.atoms.end())
               cat_to(msg, " (replace ", atom.name, " with ", it->id, ')');
           }
-          topo->err(msg);
+          topo->logger.err(msg);
         }
       }
     }
@@ -1024,7 +1017,7 @@ prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
         // check for missing altloc
         for (auto atom = res.atoms.begin(); atom + 1 < res.atoms.end(); ++atom)
           if (atom->name == (atom + 1)->name && atom->altloc == '\0')
-            topo->err("missing altloc in " + atom_str(chain_info.chain_ref, *ri.res, *atom));
+            topo->logger.err("missing altloc in ", atom_str(chain_info.chain_ref, *ri.res, *atom));
       }
 
   // for atoms with ad-hoc links, for now we don't want hydrogens
@@ -1062,15 +1055,11 @@ prepare_topology(Structure& st, MonLib& monlib, size_t model_index,
         vector_remove_if(res.atoms, [](Atom& a) { return a.is_hydrogen() && a.occ == 0; });
 
     // disable warnings here, so they are not printed twice
-    std::streambuf *warnings_orig = nullptr;
-    if (topo->warnings)
-      warnings_orig = topo->warnings->rdbuf(nullptr);
+    topo->logger.suspended = true;
     // re-set restraints and indices
     topo->apply_all_restraints(monlib);
     topo->create_indices();
-    // re-enable warnings
-    if (warnings_orig)
-      topo->warnings->rdbuf(warnings_orig);
+    topo->logger.suspended = false;
   }
 
   assign_serial_numbers(model);