Fix and improve last extend refactoring bits

src/ast.hpp

@@ -228,8 +228,8 @@ namespace Sass {
     void clear()            { return elements_.clear(); }
     T& last()               { return elements_.back(); }
     T& first()              { return elements_.front(); }
-    const T last() const    { return elements_.back(); }
-    const T first() const   { return elements_.front(); }
+    const T& last() const   { return elements_.back(); }
+    const T& first() const  { return elements_.front(); }
 
     bool operator== (const Vectorized<T>& rhs) const {
       // Abort early if sizes do not match

src/ast_helpers.hpp

@@ -5,6 +5,7 @@
 // __EXTENSIONS__ fix on Solaris.
 #include "sass.hpp"
 #include <algorithm>
+#include <functional>
 #include "util_string.hpp"
 
 namespace Sass {

src/ast_sel_weave.cpp

@@ -587,7 +587,7 @@ namespace Sass {
         groups1, groups2, {}, cmpChunkForEmptySequence);
 
     // Append chunks with inner arrays flattened
-    choices.emplace_back(std::move(flattenInner(chunks)));
+    choices.emplace_back(flattenInner(chunks));
 
     // append all trailing selectors to choices
     std::move(std::begin(trails), std::end(trails),

src/expand.cpp

@@ -677,7 +677,7 @@ namespace Sass {
           error("complex selectors may not be extended.", complex->pstate(), traces);
         }
 
-        if (auto compound = complex->first()->getCompound()) {
+        if (const CompoundSelector* compound = complex->first()->getCompound()) {
 
           if (compound->length() != 1) {
 
@@ -689,13 +689,13 @@ namespace Sass {
             // Make this an error once deprecation is over
             for (SimpleSelectorObj simple : compound->elements()) {
               // Pass every selector we ever see to extender (to make them findable for extend)
-              ctx.extender.addExtension(selector(), simple, e, mediaStack.back());
+              ctx.extender.addExtension(selector(), simple, mediaStack.back(), e->isOptional());
             }
 
           }
           else {
             // Pass every selector we ever see to extender (to make them findable for extend)
-            ctx.extender.addExtension(selector(), compound->first(), e, mediaStack.back());
+            ctx.extender.addExtension(selector(), compound->first(), mediaStack.back(), e->isOptional());
           }
 
         }

src/extender.cpp

@@ -47,8 +47,8 @@ namespace Sass {
   // ##########################################################################
   SelectorListObj Extender::extend(
     SelectorListObj& selector,
-    SelectorListObj& source,
-    SelectorListObj& targets,
+    const SelectorListObj& source,
+    const SelectorListObj& targets,
     Backtraces& traces)
   {
     return extendOrReplace(selector, source, targets, ExtendMode::TARGETS, traces);
@@ -60,8 +60,8 @@ namespace Sass {
   // ##########################################################################
   SelectorListObj Extender::replace(
     SelectorListObj& selector,
-    SelectorListObj& source,
-    SelectorListObj& targets,
+    const SelectorListObj& source,
+    const SelectorListObj& targets,
     Backtraces& traces)
   {
     return extendOrReplace(selector, source, targets, ExtendMode::REPLACE, traces);
@@ -73,9 +73,9 @@ namespace Sass {
   // ##########################################################################
   SelectorListObj Extender::extendOrReplace(
     SelectorListObj& selector,
-    SelectorListObj& source,
-    SelectorListObj& targets,
-    ExtendMode mode,
+    const SelectorListObj& source,
+    const SelectorListObj& targets,
+    const ExtendMode mode,
     Backtraces& traces)
   {
     ExtSelExtMapEntry extenders;
@@ -87,15 +87,16 @@ namespace Sass {
 
     for (auto complex : targets->elements()) {
 
-      if (complex->length() != 1) {
-        // throw "can't extend complex selector $complex."
-      }
+      // This seems superfluous, check is done before!?
+      // if (complex->length() != 1) {
+      //   error("complex selectors may not be extended.", complex->pstate(), traces);
+      // }
 
-      if (auto compound = complex->first()->getCompound()) {
+      if (const CompoundSelector* compound = complex->first()->getCompound()) {
 
         ExtSelExtMap extensions;
 
-        for (auto simple : compound->elements()) {
+        for (const SimpleSelectorObj& simple : compound->elements()) {
           extensions.insert(std::make_pair(simple, extenders));
         }
 
@@ -287,20 +288,19 @@ namespace Sass {
   // Note: this function could need some logic cleanup
   // ##########################################################################
   void Extender::addExtension(
-    SelectorListObj& extender,
-    SimpleSelectorObj& target,
-    // get get passed a pointer
-    ExtendRuleObj extend,
-    CssMediaRuleObj& mediaQueryContext)
+    const SelectorListObj& extender,
+    const SimpleSelectorObj& target,
+    const CssMediaRuleObj& mediaQueryContext,
+    bool is_optional)
   {
 
     auto rules = selectors.find(target);
     bool hasRule = rules != selectors.end();
 
     ExtSelExtMapEntry newExtensions;
 
-    auto existingExtensions = extensionsByExtender.find(target);
-    bool hasExistingExtensions = existingExtensions != extensionsByExtender.end();
+    // ToDo: we check this here first and fetch the same? item again after the loop!?
+    bool hasExistingExtensions = extensionsByExtender.find(target) != extensionsByExtender.end();
 
     ExtSelExtMapEntry& sources = extensions[target];
 
@@ -309,7 +309,7 @@ namespace Sass {
       Extension state(complex);
       // ToDo: fine-tune public API
       state.target = target;
-      state.isOptional = extend->isOptional();
+      state.isOptional = is_optional;
       state.mediaContext = mediaQueryContext;
 
       if (sources.hasKey(complex)) {
@@ -349,12 +349,15 @@ namespace Sass {
 
     ExtSelExtMap newExtensionsByTarget;
     newExtensionsByTarget.insert(std::make_pair(target, newExtensions));
-    existingExtensions = extensionsByExtender.find(target);
-    if (hasExistingExtensions && !existingExtensions->second.empty()) {
-      auto additionalExtensions =
-        extendExistingExtensions(existingExtensions->second, newExtensionsByTarget);
-      if (!additionalExtensions.empty()) {
-        mapCopyExts(newExtensionsByTarget, additionalExtensions);
+    // ToDo: do we really need to fetch again (see top off fn) 
+    auto existingExtensions = extensionsByExtender.find(target);
+    if (existingExtensions != extensionsByExtender.end()) {
+      if (hasExistingExtensions && !existingExtensions->second.empty()) {
+        auto additionalExtensions =
+          extendExistingExtensions(existingExtensions->second, newExtensionsByTarget);
+        if (!additionalExtensions.empty()) {
+          mapCopyExts(newExtensionsByTarget, additionalExtensions);
+        }
       }
     }
 
@@ -410,30 +413,32 @@ namespace Sass {
   // ##########################################################################
   ExtSelExtMap Extender::extendExistingExtensions(
     // Taking in a reference here makes MSVC debug stuck!?
-    const std::vector<Extension> oldExtensions,
-    ExtSelExtMap& newExtensions)
+    const std::vector<Extension>& oldExtensions,
+    const ExtSelExtMap& newExtensions)
   {
 
     ExtSelExtMap additionalExtensions;
 
-    for (Extension extension : oldExtensions) {
+    // During the loop `oldExtensions` vector might be changed.
+    // Callers normally pass this from `extensionsByExtender` and
+    // that points back to the `sources` vector from `extensions`.
+    for (size_t i = 0, iL = oldExtensions.size(); i < iL; i += 1) {
+      const Extension& extension = oldExtensions[i];
       ExtSelExtMapEntry& sources = extensions[extension.target];
-      std::vector<ComplexSelectorObj> selectors;
-
-      selectors = extendComplex(
+      std::vector<ComplexSelectorObj> selectors(extendComplex(
         extension.extender,
         newExtensions,
         extension.mediaContext
-      );
+      ));
 
       if (selectors.empty()) {
         continue;
       }
 
       // ToDo: "catch" error from extend
 
-      bool first = false;
-      bool containsExtension = ObjEqualityFn(selectors.front(), extension.extender);
+      bool first = false, containsExtension =
+        ObjEqualityFn(selectors.front(), extension.extender);
       for (const ComplexSelectorObj& complex : selectors) {
         // If the output contains the original complex 
         // selector, there's no need to recreate it.
@@ -442,11 +447,11 @@ namespace Sass {
           continue;
         }
 
-        Extension withExtender = extension.withExtender(complex);
+        const Extension withExtender =
+          extension.withExtender(complex);
         if (sources.hasKey(complex)) {
-          Extension source = sources.get(complex);
           sources.insert(complex, mergeExtension(
-            source, withExtender));
+            sources.get(complex), withExtender));
         }
         else {
           sources.insert(complex, withExtender);
@@ -527,7 +532,7 @@ namespace Sass {
   // ##########################################################################
   std::vector<ComplexSelectorObj> Extender::extendComplex(
     // Taking in a reference here makes MSVC debug stuck!?
-    const ComplexSelectorObj complex,
+    const ComplexSelectorObj& complex,
     const ExtSelExtMap& extensions,
     const CssMediaRuleObj& mediaQueryContext)
   {
@@ -656,7 +661,7 @@ namespace Sass {
   // ##########################################################################
   Extension Extender::extensionForCompound(
     // Taking in a reference here makes MSVC debug stuck!?
-    const std::vector<SimpleSelectorObj> simples) const
+    const std::vector<SimpleSelectorObj>& simples) const
   {
     CompoundSelectorObj compound = SASS_MEMORY_NEW(CompoundSelector, ParserState("[ext]"));
     compound->concat(simples);

src/extender.hpp

@@ -163,17 +163,17 @@ namespace Sass {
     // ##########################################################################
     static SelectorListObj extend(
       SelectorListObj& selector,
-      SelectorListObj& source,
-      SelectorListObj& target,
+      const SelectorListObj& source,
+      const SelectorListObj& target,
       Backtraces& traces);
 
     // ##########################################################################
     // Returns a copy of [selector] with [targets] replaced by [source].
     // ##########################################################################
     static SelectorListObj replace(
       SelectorListObj& selector,
-      SelectorListObj& source,
-      SelectorListObj& target,
+      const SelectorListObj& source,
+      const SelectorListObj& target,
       Backtraces& traces);
 
     // ##########################################################################
@@ -206,11 +206,10 @@ namespace Sass {
     // within the same context. A `null` context indicates no media queries.
     // ##########################################################################
     void addExtension(
-      SelectorListObj& extender,
-      SimpleSelectorObj& target,
-      // gets passed by pointer
-      ExtendRuleObj extend,
-      CssMediaRuleObj& mediaQueryContext);
+      const SelectorListObj& extender,
+      const SimpleSelectorObj& target,
+      const CssMediaRuleObj& mediaQueryContext,
+      bool is_optional = false);
 
     // ##########################################################################
     // The set of all simple selectors in style rules handled
@@ -235,9 +234,9 @@ namespace Sass {
     // ##########################################################################
     static SelectorListObj extendOrReplace(
       SelectorListObj& selector,
-      SelectorListObj& source,
-      SelectorListObj& target,
-      ExtendMode mode,
+      const SelectorListObj& source,
+      const SelectorListObj& target,
+      const ExtendMode mode,
       Backtraces& traces);
 
     // ##########################################################################
@@ -275,8 +274,8 @@ namespace Sass {
     // ##########################################################################
     ExtSelExtMap extendExistingExtensions(
       // Taking in a reference here makes MSVC debug stuck!?
-      const std::vector<Extension> extensions,
-      ExtSelExtMap& newExtensions);
+      const std::vector<Extension>& extensions,
+      const ExtSelExtMap& newExtensions);
 
     // ##########################################################################
     // Extends [list] using [extensions].
@@ -292,7 +291,7 @@ namespace Sass {
     // ##########################################################################
     std::vector<ComplexSelectorObj> extendComplex(
       // Taking in a reference here makes MSVC debug stuck!?
-      const ComplexSelectorObj list,
+      const ComplexSelectorObj& list,
       const ExtSelExtMap& extensions,
       const CssMediaRuleObj& mediaQueryContext);
 
@@ -309,7 +308,7 @@ namespace Sass {
     // ##########################################################################
     Extension extensionForCompound(
       // Taking in a reference here makes MSVC debug stuck!?
-      const std::vector<SimpleSelectorObj> simples) const;
+      const std::vector<SimpleSelectorObj>& simples) const;
 
     // ##########################################################################
     // Extends [compound] using [extensions], and returns the

src/extension.cpp

@@ -11,7 +11,7 @@ namespace Sass {
   // ##########################################################################
   // Static function to create a copy with a new extender
   // ##########################################################################
-  Extension Extension::withExtender(ComplexSelectorObj newExtender)
+  Extension Extension::withExtender(const ComplexSelectorObj& newExtender) const
   {
     Extension extension(newExtender);
     extension.specificity = specificity;

src/extension.hpp

@@ -80,7 +80,7 @@ namespace Sass {
     // compatible with the query context for this extender.
     void assertCompatibleMediaContext(CssMediaRuleObj mediaContext, Backtraces& traces) const;
 
-    Extension withExtender(ComplexSelectorObj newExtender);
+    Extension withExtender(const ComplexSelectorObj& newExtender) const;
 
   };