Re: [lmi] Request for review

[Vadim Zeitlin]

On Mon, 16 Jan 2017 01:28:36 +0000 Greg Chicares <address@hidden> wrote:

GC> Does this patch (on top of your github pull/52) seem correct?

 It does seem correct.

GC> First of all, static_get_db_names()'s implementation really is all
GC> static, so after it has been called once, calling it again just
GC> returns a const reference to a static object. It seems wasteful
GC> and, more important, needlessly verbose to store static copies of
GC> it in two other functions.

 Yes.

GC> And static_get_short_names() sounds like static_get_db_names(), but
GC> its implementation is not all static--it does work every time it's
GC> called, and there's no appealing way to avoid that--so it should be
GC> renamed to prevent confusion, and it really does make sense to store
GC> a static copy of it in the one function that calls it.

 I see the logic of doing this too, but it still feels not completely
satisfactory to store the static vector of db_names inside the function
returning it itself, while storing the similarly static map in the function
calling the function returning it. So maybe it could be better to make
static_get_short_names() live to its name and store the static map inside
it? I don't know how appealing this way of doing it, in which the map is
initialized by defining and immediately executing a lambda, would be:

---------------------------------- >8 --------------------------------------
diff --git a/dbnames.cpp b/dbnames.cpp
index a442438..057a8ff 100644
--- a/dbnames.cpp
+++ b/dbnames.cpp
@@ -87,12 +87,16 @@ bool check_order(std::vector<db_names> const& v)

 std::map<std::string,int> static_get_short_names()
 {
-    std::map<std::string,int> m;
-    static std::vector<db_names> const names = static_get_db_names();
-    for(auto const& name: names)
+    static std::map<std::string,int> const m = []()
         {
-        m[name.ShortName] = name.Idx;
+        std::map<std::string,int> m;
+        for(auto const& name : static_get_db_names())
+            {
+            m[name.ShortName] = name.Idx;
+            }
+        return m;
         }
+        ();
     return m;
 }

---------------------------------- >8 --------------------------------------

Perhaps the diff would be more clear if we ignore the whitespace changes:
---------------------------------- >8 --------------------------------------
diff --git a/dbnames.cpp b/dbnames.cpp
index a442438..057a8ff 100644
--- a/dbnames.cpp
+++ b/dbnames.cpp
@@ -87,14 +87,18 @@ bool check_order(std::vector<db_names> const& v)

 std::map<std::string,int> static_get_short_names()
 {
+    static std::map<std::string,int> const m = []()
+        {
         std::map<std::string,int> m;
-    static std::vector<db_names> const names = static_get_db_names();
-    for(auto const& name: names)
+        for(auto const& name : static_get_db_names())
             {
             m[name.ShortName] = name.Idx;
             }
         return m;
         }
+        ();
+    return m;
+}

 } // Unnamed namespace.

---------------------------------- >8 --------------------------------------

And perhaps it would be even more clear if we used different names for the
inner and outer maps (which are both called "m" above).

 I rather like the technique of initializing variables using once-only
lambdas like this as it allows me to make more variables const and, as you
know fully well, I like const a lot. But I realize that in this particular
case it might seem a bit too complicated.

 Of course, there is also the simple and straightforward (but also, even if
lmi doesn't care about this, MT-unsafe) way of initializing this map only
once:
---------------------------------- >8 --------------------------------------
diff --git a/dbnames.cpp b/dbnames.cpp
index a442438..3c6f076 100644
--- a/dbnames.cpp
+++ b/dbnames.cpp
@@ -87,11 +87,13 @@ bool check_order(std::vector<db_names> const& v)

 std::map<std::string,int> static_get_short_names()
 {
-    std::map<std::string,int> m;
-    static std::vector<db_names> const names = static_get_db_names();
-    for(auto const& name: names)
+    static std::map<std::string,int> m;
+    if (m.empty())
         {
-        m[name.ShortName] = name.Idx;
+        for(auto const& name : static_get_db_names())
+            {
+            m[name.ShortName] = name.Idx;
+            }
         }
     return m;
 }
---------------------------------- >8 --------------------------------------

 Anyhow, to return to the initial question, your patch does seem correct, I
just think that we could do things a tiny bit more consistently here.

 Regards,
VZ