rstudio / sass

Sass compiler package for R
https://rstudio.github.io/sass/
Other
102 stars 17 forks source link

warning: loop variable ‘numerator’ creates a copy from type #122

Closed cpsievert closed 1 year ago

cpsievert commented 1 year ago

A fix for this warning has been request by CRAN (by 2023-01-25)

Version: 0.4.4
Check: whether package can be installed
Result: WARN
    Found the following significant warnings:
     src/ast_values.cpp:484:23: warning: loop variable 'numerator' creates a copy from type 'const std::string' [-Wrange-loop-construct]
     src/ast_values.cpp:486:23: warning: loop variable 'denominator' creates a copy from type 'const std::string' [-Wrange-loop-construct]
    See ‘/data/gannet/ripley/R/packages/tests-clang/sass.Rcheck/00install.out’ for details.
    * used C compiler: ‘clang version 15.0.6’
    * used C++ compiler: ‘clang version 15.0.6’
Flavor: [r-devel-linux-x86_64-fedora-clang](https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-fedora-clang/sass-00check.html)
wch commented 1 year ago

I think this change should fix it (though I haven't tested yet):

--- a/src/libsass/src/ast_values.cpp
+++ b/src/libsass/src/ast_values.cpp
@@ -481,9 +481,9 @@ namespace Sass {
   {
     if (hash_ == 0) {
       hash_ = std::hash<double>()(value_);
-      for (const auto numerator : numerators)
+      for (const auto& numerator : numerators)
         hash_combine(hash_, std::hash<sass::string>()(numerator));
-      for (const auto denominator : denominators)
+      for (const auto& denominator : denominators)
         hash_combine(hash_, std::hash<sass::string>()(denominator));
     }
     return hash_;
cpsievert commented 1 year ago

Yep, thanks, I noticed LibSass 3.6.5 addressed it in https://github.com/sass/libsass/pull/3129, so I'm thinking we just update LibSass (https://github.com/rstudio/sass/pull/123)?

wch commented 1 year ago

That's a good idea. I didn't realize that we weren't on the latest version of libsass.