[Xorp-hackers] [PATCH 1/4] xorp: Added support for uint64 type

Ben Greear greearb at candelatech.com
Tue Mar 13 10:11:35 PDT 2012


On 03/13/2012 03:49 AM, igorm at etf.rs wrote:
> From: Igor Maravic<igorm at etf.rs>
>
> In template.yy and template.ll files added support for uint64 and uin64range types. Their nodes are called
> NODE_ULONG and NODE_ULONGRANGE respectively.
>
> In range.hh added U64Range class. Does everything as U32Range, but with uint64_t instead of uint32_t variables.
>
> In template_tree_node.cc and template_tree_node.hh added ULongTemplate and ULongRangeTemplate to handle new types.
>
> Unfortunately Eclipse striped whitespaces from changed files, so they are meshed up with the changed code.

First, it is best if you can at least not mix un-related whitespace
changes with code changes.  But, I can deal with it.

It have several questions on this patch.  Please see below.

>   /**
> + * @short A linear range class (uint64_t low)..(uint64_t high)
> + *
> + * Inherits from templatized general Range<uint64_t>  class.
> + * Provides specialized constructor from string and str() method.
> + */
> +class U64Range: public Range<uint64_t>  {
> +public:
> +	/**
> +	 * Default constructor
> +	 */
> +	U64Range()				{ Range<uint64_t>::_low =
> +						Range<uint64_t>::_high = 0; }
> +
> +	/**
> +	 * Constructor	from a string.
> +	 */
> +	U64Range(const char *from_cstr) {
> +	string from_string = string(from_cstr);
> +	string::size_type delim = from_string.find("..", 0);
> +	if (delim == string::npos) {
> +	    _low = _high = strtoul(from_cstr, NULL, 10);
> +	} else if (delim>  0&&  (from_string.length() - delim>  2)) {
> +	    _low = strtoul(from_string.substr(0, delim).c_str(), NULL, 10);
> +	    _high = strtoul(from_string.substr(delim + 2, from_string.length()).c_str(), NULL, 10);
> +	} else {
> +	    xorp_throw(InvalidString, "Syntax error");
> +	}
> +    }
> +
> +    /**
> +     * Convert the range to a human-readable format.
> +     *
> +     * @return C++ string.
> +     */
> +    string str() const {
> +	ostringstream os;
> +	os<<  _low;
> +	if (_low<  _high)
> +	    os<<  ".."<<  _high;
> +	return os.str();
> +    }
> +};

The indentation for the above code  is bad.  Please fix this.
Maybe you need to configure Eclipse to always use spaces instead
of tabs?  (That seems to be the way Xorp code is written for the
most part.)

> +    /**
> +     * If ctn_type() == NODE_ULONG, then
> +     * type will be NODE_UINT, because
> +     * we read uint64 values just as uint values
> +     */
> +    if (ctn->type() == NODE_ULONG&&  type == NODE_UINT)
> +		type = NODE_ULONG;

Indent 4 spaces instead of whatever this is.

> +
>       if ((ctn->type() == NODE_TEXT)&&  (type == NODE_TEXT)) {
>   	svalue = unquote(svalue);
>       } else if ((ctn->type() == NODE_TEXT)&&  (type != NODE_TEXT)) {
>   	// We'll accept anything as text
> -    } else if ((ctn->type() == NODE_UINTRANGE)&&  (type == NODE_UINT)) {
> +    } else if (((ctn->type() == NODE_UINTRANGE)&&  (type == NODE_UINT)) ||
> +			((ctn->type() == NODE_ULONGRANGE)&&  (type == NODE_ULONG))) {
>   	// Expand a single uint to a uintrange
> +    // or a single uint64 to uint64range
>   	svalue += ".." + value;
>       } else if ((ctn->type() == NODE_IPV4RANGE)&&  (type == NODE_IPV4)) {
>   	// Expand a single IPv4 to a ipv4range

Fix the indentation above.

> @@ -228,6 +228,12 @@ TemplateTree::new_node(TemplateTreeNode* parent,
>       case NODE_UINTRANGE:
>   	ttn = new UIntRangeTemplate(*this, parent, path, varname, initializer);
>   	break;
> +    case NODE_ULONG:
> +    ttn = new ULongTemplate(*this, parent, path, varname, initializer);
> +    break;
> +    case NODE_ULONGRANGE:
> +    ttn = new ULongRangeTemplate(*this, parent, path, varname, initializer);
> +    break;
>       case NODE_INT:
>   	ttn = new IntTemplate(*this, parent, path, varname, initializer);
>   	break;

My preference is to indent after the case statement.


> +ULongTemplate::ULongTemplate(TemplateTree&  template_tree,
> +			   TemplateTreeNode* parent,
> +			   const string&  path, const string&  varname,
> +			   const string&  initializer) throw (ParseError)
> +    : TemplateTreeNode(template_tree, parent, path, varname)
> +{
> +    string error_msg;
> +
> +    if (initializer.empty())
> +	return;
> +
> +    string s = strip_quotes(initializer);
> +    if (! type_match(s, error_msg)) {
> +	error_msg = c_format("Bad ULong type value \"%s\": %s.",
> +			     initializer.c_str(), error_msg.c_str());
> +	xorp_throw(ParseError, error_msg);
> +    }
> +    _default = strtoll(s.c_str(), (char **)NULL, 10);
> +    set_has_default();
> +}
> +
> +bool
> +ULongTemplate::type_match(const string&  orig, string&  error_msg) const
> +{
> +    string s = strip_quotes(orig);
> +
> +    for (size_t i = 0; i<  s.length(); i++) {
> +	if (s[i]<  '0' || s[i]>  '9') {
> +	    if (s[i]=='-') {
> +		error_msg = "value cannot be negative";
> +	    } else if (s[i]=='.') {
> +		error_msg = "value must be an integer";
> +	    } else {
> +		error_msg = "value must be numeric";
> +	    }
> +	    return false;
> +	}
> +    }
> +    return true;
> +}

Maybe more tabs instead of spaces or something?  Indentation
looks funny to me.

> +
> +string
> +ULongTemplate::default_str() const
> +{
> +    return c_format("%lu", _default);
> +}
> +
> +
> +/**************************************************************************
> + * ULongRangeTemplate
> + **************************************************************************/
> +
> +ULongRangeTemplate::ULongRangeTemplate(TemplateTree&  template_tree,
> +			   TemplateTreeNode* parent,
> +			   const string&  path, const string&  varname,
> +			   const string&  initializer) throw (ParseError)
> +    : TemplateTreeNode(template_tree, parent, path, varname),
> +      _default(NULL)
> +{
> +    string error_msg;
> +
> +    if (initializer.empty())
> +	return;
> +
> +    try {
> +	_default = new U64Range(initializer.c_str());
> +    } catch (InvalidString) {
> +	error_msg = c_format("Bad U64Range type value \"%s\".",
> +			     initializer.c_str());
> +	xorp_throw(ParseError, error_msg);
> +    }
> +    set_has_default();
> +}
> +
> +ULongRangeTemplate::~ULongRangeTemplate()
> +{
> +    if (_default != NULL)
> +	delete _default;
> +}
> +
> +string
> +ULongRangeTemplate::default_str() const
> +{
> +    if (_default != NULL)
> +	return _default->str();
> +
> +    return "";
> +}
> +
> +bool
> +ULongRangeTemplate::type_match(const string&  s, string&  error_msg) const
> +{
> +    string tmp = strip_quotes(s);
> +
> +    if (tmp.empty()) {
> +	error_msg = "value must be a valid range of unsigned 64-bit integers";
> +	return false;
> +    }
> +
> +    try {
> +	U64Range* u64range = new U64Range(tmp.c_str());
> +	delete u64range;
> +    } catch (InvalidString) {
> +	error_msg = "value must be a valid range of unsigned 64-bit integers";
> +	return false;
> +    }
> +    return true;
> +}

More indentation issues above.

> @@ -42,22 +42,24 @@ enum TTNodeType {
>       NODE_VOID		= 0,
>       NODE_TEXT		= 1,
>       NODE_UINT		= 2,
> -    NODE_INT		= 3,
> -    NODE_BOOL		= 4,
> -    NODE_TOGGLE		= 4,
> -    NODE_IPV4		= 5,
> -    NODE_IPV4NET	= 6,
> -    NODE_IPV6		= 7,
> -    NODE_IPV6NET	= 8,
> -    NODE_MACADDR	= 9,
> -    NODE_URL_FILE	= 10,
> -    NODE_URL_FTP	= 11,
> -    NODE_URL_HTTP	= 12,
> -    NODE_URL_TFTP	= 13,
> -    NODE_ARITH		= 14,
> -    NODE_UINTRANGE	= 15,
> -    NODE_IPV4RANGE	= 16,
> -    NODE_IPV6RANGE	= 17
> +    NODE_ULONG		= 3,
> +    NODE_INT		= 4,
> +    NODE_BOOL		= 5,
> +    NODE_TOGGLE		= 5,
> +    NODE_IPV4		= 6,
> +    NODE_IPV4NET	= 7,
> +    NODE_IPV6		= 8,
> +    NODE_IPV6NET	= 9,
> +    NODE_MACADDR	= 10,
> +    NODE_URL_FILE	= 11,
> +    NODE_URL_FTP	= 12,
> +    NODE_URL_HTTP	= 13,
> +    NODE_URL_TFTP	= 14,
> +    NODE_ARITH		= 15,
> +    NODE_UINTRANGE	= 16,
> +    NODE_ULONGRANGE	= 17,
> +    NODE_IPV4RANGE	= 18,
> +    NODE_IPV6RANGE	= 19
>   };

Why are you adding in the middle?  It would seem to
be better to add at the end to me..less code change
and less risk of something weird happening??


Please fix this up and re-submit.

Thanks,
Ben

-- 
Ben Greear <greearb at candelatech.com>
Candela Technologies Inc  http://www.candelatech.com



More information about the Xorp-hackers mailing list