Re: Simplify ly_symbol2scm (issue 363750043 by address@hidden)

[dak]

Reviewers: Dan Eble,


https://codereview.appspot.com/363750043/diff/1/lily/include/lily-guile-macros.hh
File lily/include/lily-guile-macros.hh (right):

https://codereview.appspot.com/363750043/diff/1/lily/include/lily-guile-macros.hh#newcode95
lily/include/lily-guile-macros.hh:95: value;
                                 \
On 2018/08/04 12:49:42, Dan Eble wrote:
> This is new to me.  It is a GCC extension, according to
> https://stackoverflow.com/a/1635559.  I'd probably add a TODO comment
that
> suggests converting this to a lambda once we can depend on C++11.

__builtin_constant_p would not work in a lambda expression, so the
caching for constants would not work, making the code equivalent (though
much more complicated) to the #else path below.

So you'd need to create a top-level ?: expression predicated on
__built_in_constant_p and fork out into an immediately called lambda
expression in the ? branch for the only purpose of creating a static
variable.

And then check that the optimizer does not do anything stupid.

Note that we already _could_ do the ?: wrapper, turning this into
(__built_in_constant_p (x) ? ({ static SCM cached = ...; cached})
                            : scm_or_str2symbol (x))

You propose using instead
(__builtin_constant_p (x) ? [] (SCM arg){ static SCM cached = ...;
return cached;}()
                          : scm_or_str2symbol (x));

Frankly, it doesn't read better to me and one would have to check pretty
carefully that it does not generate worse code since this memoization is
used a lot in frequently travelled code paths.

There are a lot of other occasions for using lambda that have less
questionable payoff.

The core "if" or equivalently ?: has to be a macro or
__builtin_constant_p will not be able to do its part of the job.  And
__builtin_constant_p, a mandatory part of this optimization, is already
a GCC extension so there is absolutely no gain for going to C++11 here.

I'm actually sympathetic to going the ?: route which I personally find
more readable than introducing an extra "value" variable, and if "x"
contains "value" in some respect, the extra hygiene would pay off.

I'll do that and test the results.  But C++11 here would not help in any
manner I see.

Description:
Simplify ly_symbol2scm

Please review this at https://codereview.appspot.com/363750043/

Affected files (+4, -5 lines):
  M lily/include/lily-guile-macros.hh


Index: lily/include/lily-guile-macros.hh
diff --git a/lily/include/lily-guile-macros.hh  
b/lily/include/lily-guile-macros.hh
index  
869a52af327c137a72b634e8ac837630f94acfd8..97687871bfba424a90eca5f5fef50e0fdb744a76  
100644
--- a/lily/include/lily-guile-macros.hh
+++ b/lily/include/lily-guile-macros.hh
@@ -83,13 +83,12 @@ scm_or_str2symbol (SCM s)
  */
 #define ly_symbol2scm(x)                                                \
   ({                                                                    \
-    static SCM cached;                                                  \
-    /* We store this one locally, since G++ -O2 fucks up else */        \
-    SCM value = cached;                                                 \
+    SCM value;                                                          \
     if (__builtin_constant_p ((x)))                                     \
       {                                                                 \
-        if (!SCM_UNPACK (cached))                                       \
-          value = cached = scm_gc_protect_object (scm_or_str2symbol (x)); \
+        static SCM cached                                               \
+          = scm_gc_protect_object (scm_or_str2symbol (x));              \
+        value = cached;                                                 \
       }                                                                 \
     else                                                                \
       value = scm_or_str2symbol (x);                                    \