Re: [PATCH 1/1] Adding enumeration support to statement pp

[Jonathan Lebon <jlebon at redhat dot com>]

Hey Brian,

Looking good! Had much less trouble applying your patch this time.
Just a few more things:

> @@ -1777,7 +1777,7 @@ get_funcs_in_srcfile(base_func_info_map_t& funcs,
>  
>  template<> void
>  dwflpp::iterate_over_srcfile_lines<void>(char const * srcfile,
> -                                         int linenos[2],
> +                                         std::vector<int> linenos,
>                                           enum lineno_t lineno_type,
>                                           base_func_info_map_t& funcs,
>                                           void (* callback) (Dwarf_Addr,

> @@ -1858,7 +1858,7 @@ template<> void
>  dwflpp::iterate_over_labels<void>(Dwarf_Die *begin_die,
>                                    const string& sym,
>                                    const base_func_info& function,
> -                                  int linenos[2],
> +                                  std::vector<int> linenos,
>                                    enum lineno_t lineno_type,
>                                    void *data,
>                                    void (* callback)(const base_func_info&,

Since we're not modifying the vector in these functions, let's pass them as
const vector<int>& to signal this. Also minor nitpick, you only need the
'std::' prefix in dwflpp.h.

> @@ -1916,8 +1916,8 @@ dwflpp::iterate_over_labels<void>(Dwarf_Die *begin_die,
>                          matches_lineno = dline == linenos[0];
>                        else if (lineno_type == RELATIVE)
>                          matches_lineno = dline == linenos[0] +
>                          function.decl_line;
> -                      else if (lineno_type == RANGE)
> -                        matches_lineno = (linenos[0] <= dline && dline <= linenos[1]);
> +                      else if (lineno_type == ENUMERATED)
> +                        matches_lineno = (std::binary_search(linenos.begin(), linenos.end(), dline));
>                        else // WILDCARD
>                          matches_lineno = true;

Here too, we don't need the 'std::' prefix.

> @@ -1086,9 +1086,9 @@ void
>  dwarf_query::parse_function_spec(const string & spec)
>  {
>    lineno_type = ABSOLUTE;
> -  linenos[0] = linenos[1] = 0;
> -
> -  size_t src_pos, line_pos, dash_pos, scope_pos;
> +  size_t src_pos, line_pos, scope_pos;
> +  linenos.push_back(0);
> +  linenos.push_back(0);
>  
>    // look for named scopes
>    scope_pos = spec.rfind("::");

I'm thinking we should probably abandon this 'seeding'. It made sense to
initialize them to safe values back when it was arrays, but it'd probably
be nicer now to push_back() the actual lineno once/if we get it.

> @@ -1135,19 +1135,28 @@ dwarf_query::parse_function_spec(const string & spec)
>            if (lineno_type != WILDCARD)
>              try
>                {
> -                // try to parse either N or N-M
> -                dash_pos = spec.find('-', line_pos + 1);
> -                if (dash_pos == string::npos)
> -                  linenos[0] = linenos[1] = lex_cast<int>(spec.substr(line_pos + 1));
> +                // try to parse N, N-M, or N,M,O,P, or combination thereof...
> +                if (spec.find_first_of(",-", line_pos + 1) != string::npos)
> +                  {
> +                    lineno_type = ENUMERATED;
> +                    linenos.clear();
> +                    vector<string> sub_specs,ranges;
> +                    tokenize(spec.substr(line_pos + 1), sub_specs, ",");
> +                    vector<string>::const_iterator line_spec;
> +                    for (line_spec = sub_specs.begin(); line_spec != sub_specs.end(); ++line_spec)
> +                      {
> +                        ranges.clear();
> +                        tokenize(*line_spec, ranges, "-");
> +                        if (ranges.size() > 1)
> +                            for (int i = lex_cast<int>(ranges.front()); i <= lex_cast<int>(ranges.back()); i++)
> +                                linenos.push_back(i);
>                          else
> -                  {
> -                    lineno_type = RANGE;
> -                    linenos[0] = lex_cast<int>(spec.substr(line_pos + 1,
> -                                                        dash_pos - line_pos
> - 1));
> -                    linenos[1] = lex_cast<int>(spec.substr(dash_pos + 1));
> -                    if (linenos[0] > linenos[1])
> -                      throw runtime_error("bad range");
> +                            linenos.push_back(lex_cast<int>(ranges.at(0)));
>                        }
> +                    std::sort(linenos.begin(), linenos.end());
> +                  }
> +                else
> +                  linenos[0] = linenos[1] = lex_cast<int>(spec.substr(line_pos + 1));
>                }
>              catch (runtime_error & exn)
>                {

Why not have the ranges vector be declared in the for-loop instead? Then there's no
clearing required at every iteration. (Also note if we stop the 'seeding' we won't
need to clear the main linenos vector here either).

> @@ -1188,8 +1197,22 @@ dwarf_query::parse_function_spec(const string & spec)
>                clog << "+" << linenos[0];
>                break;
>  
> -            case RANGE:
> -              clog << linenos[0] << " - " << linenos[1];
> +            case ENUMERATED:
> +              {
> +                vector<int>::const_iterator linenos_it,range_it;
> +                for (linenos_it = linenos.begin(); linenos_it != linenos.end(); ++linenos_it)
> +                  {
> +                    while ((range_it+1) != linenos.end() && *range_it + 1 == *(range_it+1))
> +                        ++range_it;
> +                    if (linenos_it == range_it)
> +                        clog << *linenos_it;
> +                    else
> +                        clog << *linenos_it << "-" << *range_it;
> +                    if (range_it + 1 != linenos.end())
> +                      clog << ",";
> +                    linenos_it = range_it;
> +                  }
> +                }
>                break;
>  
>              case WILDCARD:

This does not work. I received a SEGSIGV when trying it out. I suppose it is
because the range_it iterator is never initialized. Although even after fixing
that, it was stuck in a loop printing the same number over and over.

> diff --git a/testsuite/systemtap.base/statement.exp
> b/testsuite/systemtap.base/statement.exp
> index a8a1f87..546450a 100644
> --- a/testsuite/systemtap.base/statement.exp
> +++ b/testsuite/systemtap.base/statement.exp

Testing looks good. Passed all the tests here!


Cheers,

Jonathan