Change attribute syntax in script to come after variable in set and for (set x [...], for x [...])

2 files changed, 53 insertions, 36 deletions

diff --git a/libbuild2/script/parser.cxx b/libbuild2/script/parser.cxx
index 87a51d8..2a213ab 100644
--- a/libbuild2/script/parser.cxx
+++ b/libbuild2/script/parser.cxx
@@ -2497,28 +2497,36 @@ namespace build2
                     if (!scan.more ())
                       fail (ll) << "for: missing variable name";
 
-                    // Either attributes or variable name.
-                    //
-                    string a (scan.next ());
-                    const string* ats (!scan.more () ? nullptr : &a);
-                    string vname (!scan.more () ? move (a) : scan.next ());
-
-                    if (scan.more ())
-                      fail (ll) << "for: unexpected argument '"
-                                << scan.next () << "'";
-
-                    if (ats != nullptr && ats->empty ())
-                      fail (ll) << "for: empty variable attributes";
-
+                    string vname (scan.next ());
                     if (vname.empty ())
                       fail (ll) << "for: empty variable name";
 
+                    // Detect patterns analogous to parse_variable_name() (so
+                    // we diagnose `for x[string]`).
+                    //
+                    if (vname.find_first_of ("[*?") != string::npos)
+                      fail (ll) << "for: expected variable name instead of "
+                                << vname;
+
                     // Let's also diagnose the `... | for x:...` misuse which
                     // can probably be quite common.
                     //
                     if (vname.find (':') != string::npos)
                       fail (ll) << "for: ':' after variable name";
 
+                    string attrs;
+                    if (scan.more ())
+                    {
+                      attrs = scan.next ();
+
+                      if (attrs.empty ())
+                        fail (ll) << "for: empty variable attributes";
+
+                      if (scan.more ())
+                        fail (ll) << "for: unexpected argument '"
+                                  << scan.next () << "'";
+                    }
+
                     stream_reader sr (
                       move (in), pipe,
                       !ops.newline (), ops.newline (), ops.exact (),
@@ -2544,7 +2552,7 @@ namespace build2
                       //
                       env.set_variable (vname,
                                         names {name (move (*s))},
-                                        ats != nullptr ? *ats : empty_string,
+                                        attrs,
                                         ll);
 
                       // Find the construct end, if it is not found yet.
@@ -2582,15 +2590,9 @@ namespace build2
             }
           case line_type::cmd_for_args:
             {
-              // Parse the variable name with the potential attributes.
+              // Parse the variable name.
               //
-              next_with_attributes (t, tt);
-              attributes_push (t, tt);
-
-              // @@ TMP Currently we assume that these are the value (rather
-              //        than the variable) attributes.
-              //
-              attributes val_attrs (attributes_pop ());
+              next (t, tt);
 
               assert (tt == type::word && t.qtype == quote_type::unquoted);
 
@@ -2609,9 +2611,18 @@ namespace build2
                 var = &var_pool->insert (move (vn));
               }
 
-              next (t, tt); // Skip the colon.
+              // Parse the potential element attributes and skip the colon.
+              //
+              next_with_attributes (t, tt);
+              attributes_push (t, tt);
+
               assert (tt == type::colon);
 
+              // Save element attributes so that we can inject them on each
+              // iteration.
+              //
+              attributes val_attrs (attributes_pop ());
+
               // Parse the value with the potential attributes.
               //
               // Note that we don't really need to change the mode since we
diff --git a/libbuild2/script/run.cxx b/libbuild2/script/run.cxx
index b7f3314..ca04443 100644
--- a/libbuild2/script/run.cxx
+++ b/libbuild2/script/run.cxx
@@ -1171,7 +1171,7 @@ namespace build2
 
     // The set pseudo-builtin: set variable from the stdin input.
     //
-    // set [-e|--exact] [(-n|--newline)|(-w|--whitespace)] [<attr>] <var>
+    // set [-e|--exact] [(-n|--newline)|(-w|--whitespace)] <var> [<attr>]
     //
     static void
     set_builtin (environment& env,
@@ -1195,18 +1195,27 @@ namespace build2
         if (!scan.more ())
           fail (ll) << "set: missing variable name";
 
-        string a (scan.next ()); // Either attributes or variable name.
-        const string* ats (!scan.more () ? nullptr : &a);
-        string vname (!scan.more () ? move (a) : scan.next ());
+        string vname (scan.next ());
+        if (vname.empty ())
+          fail (ll) << "set: empty variable name";
 
+        // Detect patterns analogous to parser::parse_variable_name() (so we
+        // diagnose `set x[string]`).
+        //
+        if (vname.find_first_of ("[*?") != string::npos)
+          fail (ll) << "set: expected variable name instead of " << vname;
+
+        string attrs;
         if (scan.more ())
-          fail (ll) << "set: unexpected argument '" << scan.next () << "'";
+        {
+          attrs = scan.next ();
 
-        if (ats != nullptr && ats->empty ())
-          fail (ll) << "set: empty variable attributes";
+          if (attrs.empty ())
+            fail (ll) << "set: empty variable attributes";
 
-        if (vname.empty ())
-          fail (ll) << "set: empty variable name";
+          if (scan.more ())
+            fail (ll) << "set: unexpected argument '" << scan.next () << "'";
+        }
 
         stream_reader sr (move (in), pipe,
                           ops.whitespace (), ops.newline (), ops.exact (),
@@ -1220,10 +1229,7 @@ namespace build2
         for (optional<string> s; (s = sr.next ()); )
           ns.emplace_back (move (*s));
 
-        env.set_variable (move (vname),
-                          move (ns),
-                          ats != nullptr ? *ats : empty_string,
-                          ll);
+        env.set_variable (move (vname), move (ns), attrs, ll);
       }
       catch (const io_error& e)
       {