Skip to content

Commit

Permalink
Avoid modifying Expression in ast_node_to_sass_value
Browse files Browse the repository at this point in the history 
Previously, the argument to `ast_node_to_sass_value` was a `const Expression_Ptr`.

The `const` there was useless, as it didn't mean the constness of Expression but merely that `val` pointer could not be reassigned within the function (due to how pointer `typedef` and `const` interacts).

Assuming the intended argument type was `const Expression_Ptr *`, I've changed it to `Expression_Ptr_Const`.

This required splitting `to{RGBA,HSLA}(bool)` into separate functions, so that the one that always copies could be marked as `const`.
  • Loading branch information
@glebm @xzyfer
glebm authored and xzyfer committed Mar 18, 2019
1 parent bc55151 commit 20b91ef
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 32 deletions.
5 changes: 4 additions & 1 deletion src/ast2c.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ namespace Sass {
{ return sass_make_color(c->r(), c->g(), c->b(), c->a()); }

union Sass_Value* AST2C::operator()(Color_HSLA_Ptr c)
{ return operator()(c->toRGBA()); }
{
Color_RGBA_Obj rgba = c->copyAsRGBA();
return operator()(rgba.ptr());
}

union Sass_Value* AST2C::operator()(String_Constant_Ptr s)
{
Expand Down
12 changes: 6 additions & 6 deletions src/ast_values.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ namespace Sass {
return hash_;
}

Color_HSLA_Ptr Color_RGBA::toHSLA(bool copy)
Color_HSLA_Ptr Color_RGBA::copyAsHSLA() const
{

// Algorithm from http://en.wikipedia.org/wiki/wHSL_and_HSV#Conversion_from_RGB_to_HSL_or_HSV
Expand Down Expand Up @@ -592,9 +592,9 @@ namespace Sass {
);
}

Color_RGBA_Ptr Color_RGBA::toRGBA(bool copy)
Color_RGBA_Ptr Color_RGBA::copyAsRGBA() const
{
return copy ? SASS_MEMORY_COPY(this) : this;
return SASS_MEMORY_COPY(this);
}

/////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -649,7 +649,7 @@ namespace Sass {
return m1;
}

Color_RGBA_Ptr Color_HSLA::toRGBA(bool copy)
Color_RGBA_Ptr Color_HSLA::copyAsRGBA() const
{

double h = absmod(h_ / 360.0, 1.0);
Expand All @@ -671,9 +671,9 @@ namespace Sass {
);
}

Color_HSLA_Ptr Color_HSLA::toHSLA(bool copy)
Color_HSLA_Ptr Color_HSLA::copyAsHSLA() const
{
return copy ? SASS_MEMORY_COPY(this) : this;
return SASS_MEMORY_COPY(this);
}

/////////////////////////////////////////////////////////////////////////
Expand Down
23 changes: 17 additions & 6 deletions src/ast_values.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,11 @@ namespace Sass {

bool operator== (const Expression& rhs) const override;

virtual Color_RGBA_Ptr toRGBA(bool copy = false) = 0;
virtual Color_HSLA_Ptr toHSLA(bool copy = false) = 0;
virtual Color_RGBA_Ptr copyAsRGBA() const = 0;
virtual Color_RGBA_Ptr toRGBA() = 0;

virtual Color_HSLA_Ptr copyAsHSLA() const = 0;
virtual Color_HSLA_Ptr toHSLA() = 0;

ATTACH_VIRTUAL_AST_OPERATIONS(Color)
};
Expand All @@ -273,8 +276,12 @@ namespace Sass {
static std::string type_name() { return "color"; }

size_t hash() const override;
Color_RGBA_Ptr toRGBA(bool copy = false) override;
Color_HSLA_Ptr toHSLA(bool copy = false) override;

Color_RGBA_Ptr copyAsRGBA() const override;
Color_RGBA_Ptr toRGBA() override { return this; }

Color_HSLA_Ptr copyAsHSLA() const override;
Color_HSLA_Ptr toHSLA() override { return copyAsHSLA(); }

bool operator== (const Expression& rhs) const override;

Expand All @@ -297,8 +304,12 @@ namespace Sass {
static std::string type_name() { return "color"; }

size_t hash() const override;
Color_RGBA_Ptr toRGBA(bool copy = false) override;
Color_HSLA_Ptr toHSLA(bool copy = false) override;

Color_RGBA_Ptr copyAsRGBA() const override;
Color_RGBA_Ptr toRGBA() override { return copyAsRGBA(); }

Color_HSLA_Ptr copyAsHSLA() const override;
Color_HSLA_Ptr toHSLA() override { return this; }

bool operator== (const Expression& rhs) const override;

Expand Down
28 changes: 14 additions & 14 deletions src/fn_colors.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ namespace Sass {
{
Color_Ptr col = ARG("$color", Color);
double degrees = ARGVAL("$degrees");
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->h(absmod(copy->h() + degrees, 360.0));
return copy.detach();
}
Expand All @@ -287,7 +287,7 @@ namespace Sass {
{
Color_Ptr col = ARG("$color", Color);
double amount = DARG_U_PRCT("$amount");
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->l(clip(copy->l() + amount, 0.0, 100.0));
return copy.detach();

Expand All @@ -298,7 +298,7 @@ namespace Sass {
{
Color_Ptr col = ARG("$color", Color);
double amount = DARG_U_PRCT("$amount");
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->l(clip(copy->l() - amount, 0.0, 100.0));
return copy.detach();
}
Expand All @@ -313,7 +313,7 @@ namespace Sass {

Color_Ptr col = ARG("$color", Color);
double amount = DARG_U_PRCT("$amount");
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->s(clip(copy->s() + amount, 0.0, 100.0));
return copy.detach();
}
Expand All @@ -323,7 +323,7 @@ namespace Sass {
{
Color_Ptr col = ARG("$color", Color);
double amount = DARG_U_PRCT("$amount");
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->s(clip(copy->s() - amount, 0.0, 100.0));
return copy.detach();
}
Expand All @@ -338,7 +338,7 @@ namespace Sass {
}

Color_Ptr col = ARG("$color", Color);
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->s(0.0); // just reset saturation
return copy.detach();
}
Expand All @@ -351,7 +351,7 @@ namespace Sass {
BUILT_IN(complement)
{
Color_Ptr col = ARG("$color", Color);
Color_HSLA_Obj copy = col->toHSLA(true);
Color_HSLA_Obj copy = col->copyAsHSLA();
copy->h(absmod(copy->h() - 180.0, 360.0));
return copy.detach();
}
Expand All @@ -367,7 +367,7 @@ namespace Sass {

Color_Ptr col = ARG("$color", Color);
double weight = DARG_U_PRCT("$weight");
Color_RGBA_Obj inv = col->toRGBA(true);
Color_RGBA_Obj inv = col->copyAsRGBA();
inv->r(clip(255.0 - inv->r(), 0.0, 255.0));
inv->g(clip(255.0 - inv->g(), 0.0, 255.0));
inv->b(clip(255.0 - inv->b(), 0.0, 255.0));
Expand Down Expand Up @@ -441,15 +441,15 @@ namespace Sass {
error("Cannot specify HSL and RGB values for a color at the same time for `adjust-color'", pstate, traces);
}
else if (rgb) {
Color_RGBA_Obj c = col->toRGBA(true);
Color_RGBA_Obj c = col->copyAsRGBA();
if (r) c->r(c->r() + DARG_R_BYTE("$red"));
if (g) c->g(c->g() + DARG_R_BYTE("$green"));
if (b) c->b(c->b() + DARG_R_BYTE("$blue"));
if (a) c->a(c->a() + DARG_R_FACT("$alpha"));
return c.detach();
}
else if (hsl) {
Color_HSLA_Obj c = col->toHSLA(true);
Color_HSLA_Obj c = col->copyAsHSLA();
if (h) c->h(c->h() + absmod(h->value(), 360.0));
if (s) c->s(c->s() + DARG_R_PRCT("$saturation"));
if (l) c->l(c->l() + DARG_R_PRCT("$lightness"));
Expand Down Expand Up @@ -486,7 +486,7 @@ namespace Sass {
error("Cannot specify HSL and RGB values for a color at the same time for `scale-color'", pstate, traces);
}
else if (rgb) {
Color_RGBA_Obj c = col->toRGBA(true);
Color_RGBA_Obj c = col->copyAsRGBA();
double rscale = (r ? DARG_R_PRCT("$red") : 0.0) / 100.0;
double gscale = (g ? DARG_R_PRCT("$green") : 0.0) / 100.0;
double bscale = (b ? DARG_R_PRCT("$blue") : 0.0) / 100.0;
Expand All @@ -498,7 +498,7 @@ namespace Sass {
return c.detach();
}
else if (hsl) {
Color_HSLA_Obj c = col->toHSLA(true);
Color_HSLA_Obj c = col->copyAsHSLA();
double hscale = (h ? DARG_R_PRCT("$hue") : 0.0) / 100.0;
double sscale = (s ? DARG_R_PRCT("$saturation") : 0.0) / 100.0;
double lscale = (l ? DARG_R_PRCT("$lightness") : 0.0) / 100.0;
Expand Down Expand Up @@ -540,15 +540,15 @@ namespace Sass {
error("Cannot specify HSL and RGB values for a color at the same time for `change-color'", pstate, traces);
}
else if (rgb) {
Color_RGBA_Obj c = col->toRGBA(true);
Color_RGBA_Obj c = col->copyAsRGBA();
if (r) c->r(DARG_U_BYTE("$red"));
if (g) c->g(DARG_U_BYTE("$green"));
if (b) c->b(DARG_U_BYTE("$blue"));
if (a) c->a(DARG_U_FACT("$alpha"));
return c.detach();
}
else if (hsl) {
Color_HSLA_Obj c = col->toHSLA(true);
Color_HSLA_Obj c = col->copyAsHSLA();
if (h) c->h(absmod(h->value(), 360.0));
if (s) c->s(DARG_U_PRCT("$saturation"));
if (l) c->l(DARG_U_PRCT("$lightness"));
Expand Down
12 changes: 8 additions & 4 deletions src/values.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
namespace Sass {

// convert value from C++ side to C-API
union Sass_Value* ast_node_to_sass_value (const Expression_Ptr val)
union Sass_Value* ast_node_to_sass_value (Expression_Ptr_Const val)
{
if (val->concrete_type() == Expression::NUMBER)
{
Expand All @@ -19,9 +19,13 @@ namespace Sass {
}
else if (val->concrete_type() == Expression::COLOR)
{
// ToDo: allow to also use HSLA colors!!
Color_RGBA_Obj col = Cast<Color>(val)->toRGBA();
return sass_make_color(col->r(), col->g(), col->b(), col->a());
if (Color_RGBA_Ptr_Const rgba = Cast<Color_RGBA>(val)) {
return sass_make_color(rgba->r(), rgba->g(), rgba->b(), rgba->a());
} else {
// ToDo: allow to also use HSLA colors!!
Color_RGBA_Obj col = Cast<Color>(val)->copyAsRGBA();
return sass_make_color(col->r(), col->g(), col->b(), col->a());
}
}
else if (val->concrete_type() == Expression::LIST)
{
Expand Down
2 changes: 1 addition & 1 deletion src/values.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

namespace Sass {

union Sass_Value* ast_node_to_sass_value (const Expression_Ptr val);
union Sass_Value* ast_node_to_sass_value (Expression_Ptr_Const val);
Value_Ptr sass_value_to_ast_node (const union Sass_Value* val);

}
Expand Down

0 comments on commit 20b91ef

Please sign in to comment.