Skip to content

Document util/expr.{cpp,h} [DOC-11] #2790

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions doc/assets/expr.dot
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
graph {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the graph! Only one thing: 💡 could the ID_type node been given some dots below it or something similar to indicate that it's not just a string, but a typet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the graph more complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterschrammel or @smowton, could you please check for correctness?

graph [pad="0.5", nodesep="1", ranksep="1"];
plus [ label="ID_plus", xlabel="exprt" ];
const3 [ label="ID_constant", xlabel="exprt" ];
const5 [ label="ID_constant", xlabel="exprt" ];
val3 [ label="3", xlabel="dstringt" ];
val5 [ label="5", xlabel="dstringt" ];
ID_integer [ xlabel="typet" ];
type3 [ label="ID_integer", xlabel="typet" ];
type5 [ label="ID_integer", xlabel="typet" ];
plus -- const3 [ label="sub[0]" ];
plus -- const5 [ label="sub[1]" ];
plus -- ID_integer [ label="named_sub[ID_type]" ];
const3 -- val3 [ label="sub[ID_value]" ];
const3 -- type3 [ label="sub[ID_type]" ];
const5 -- val5 [ label="sub[ID_value]" ];
const5 -- type5 [ label="sub[ID_type]" ];
}
2 changes: 1 addition & 1 deletion src/doxyfile
Original file line number Diff line number Diff line change
Expand Up @@ -2342,7 +2342,7 @@ DOT_PATH =
# command).
# This tag requires that the tag HAVE_DOT is set to YES.

DOTFILE_DIRS =
DOTFILE_DIRS = ../doc/assets/

# The MSCFILE_DIRS tag can be used to specify one or more directories that
# contain msc files that are included in the documentation (see the \mscfile
Expand Down
64 changes: 64 additions & 0 deletions src/util/expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Module: Expression Representation

Author: Daniel Kroening, [email protected]
Joel Allred, [email protected]

\*******************************************************************/

Expand All @@ -19,13 +20,20 @@ Author: Daniel Kroening, [email protected]

#include <stack>

/// Move the given argument to the end of `exprt`'s operands.
/// The argument is destroyed and mutated to a reference to a nil `irept`.
/// \param expr: `exprt` to append to the operands
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be \ref exprt instead? (But maybe doxygen treats "exprt" the same.) To be considered throughout this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are treated differently. The backward quotes only enforces monospace. Would you prefer seeing a link everywhere? I was worried it would make the source less readable while not adding much to the doxygen (where there are already links to exprt everywhere). What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have used the human readable "expression" instead of the programmer-only "exprt" (and, equally, would have said "to the end of the expression's operands" in the first sentence). But then that's all a debate about style and therefore entirely up to you - unless there is broad consensus and consistency across the entire documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no broad consensus, but I think I saw the the reverse-quoted version more frequently. So I'll leave it that way.

void exprt::move_to_operands(exprt &expr)
{
operandst &op=operands();
op.push_back(static_cast<const exprt &>(get_nil_irep()));
op.back().swap(expr);
}

/// Move the given arguments to the end of `exprt`'s operands.
/// The arguments are destroyed and mutated to a reference to a nil `irept`.
/// \param e1: first `exprt` to append to the operands
/// \param e2: second `exprt` to append to the operands
void exprt::move_to_operands(exprt &e1, exprt &e2)
{
operandst &op=operands();
Expand All @@ -38,6 +46,11 @@ void exprt::move_to_operands(exprt &e1, exprt &e2)
op.back().swap(e2);
}

/// Move the given arguments to the end of `exprt`'s operands.
/// The arguments are destroyed and mutated to a reference to a nil `irept`.
/// \param e1: first `exprt` to append to the operands
/// \param e2: second `exprt` to append to the operands
/// \param e3: third `exprt` to append to the operands
void exprt::move_to_operands(exprt &e1, exprt &e2, exprt &e3)
{
operandst &op=operands();
Expand All @@ -52,11 +65,16 @@ void exprt::move_to_operands(exprt &e1, exprt &e2, exprt &e3)
op.back().swap(e3);
}

/// Copy the given argument to the end of `exprt`'s operands.
/// \param expr: `exprt` to append to the operands
void exprt::copy_to_operands(const exprt &expr)
{
operands().push_back(expr);
}

/// Copy the given arguments to the end of `exprt`'s operands.
/// \param e1: first `exprt` to append to the operands
/// \param e2: second `exprt` to append to the operands
void exprt::copy_to_operands(const exprt &e1, const exprt &e2)
{
operandst &op=operands();
Expand All @@ -67,6 +85,10 @@ void exprt::copy_to_operands(const exprt &e1, const exprt &e2)
op.push_back(e2);
}

/// Copy the given arguments to the end of `exprt`'s operands.
/// \param e1: first `exprt` to append to the operands
/// \param e2: second `exprt` to append to the operands
/// \param e3: third `exprt` to append to the operands
void exprt::copy_to_operands(
const exprt &e1,
const exprt &e2,
Expand All @@ -81,13 +103,21 @@ void exprt::copy_to_operands(
op.push_back(e3);
}

/// Create a \ref typecast_exprt to the given type.
/// \param _type: cast destination type
/// \deprecated use constructors instead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 There is a corresponding comment in expr.h that can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

void exprt::make_typecast(const typet &_type)
{
typecast_exprt new_expr(*this, _type);

swap(new_expr);
}

/// Negate the expression.
/// Simplifications:
/// - If the expression is trivially true, make it false, and vice versa.
/// - If the expression is an `ID_not`, remove the not.
/// \deprecated use constructors instead
void exprt::make_not()
{
if(is_true())
Expand Down Expand Up @@ -116,48 +146,71 @@ void exprt::make_not()
swap(new_expr);
}

/// Return whether the expression is a constant.
/// \return True if is a constant, false otherwise
bool exprt::is_constant() const
{
return id()==ID_constant;
}

/// Return whether the expression is a constant representing `true`.
/// \return True if is a Boolean constant representing `true`, false otherwise.
bool exprt::is_true() const
{
return is_constant() &&
type().id()==ID_bool &&
get(ID_value)!=ID_false;
}

/// Return whether the expression is a constant representing `false`.
/// \return True if is a Boolean constant representing `false`, false otherwise.
bool exprt::is_false() const
{
return is_constant() &&
type().id()==ID_bool &&
get(ID_value)==ID_false;
}

/// Replace the expression by a Boolean expression representing \p value.
/// \param value: the Boolean value to give to the expression
/// \deprecated use constructors instead
void exprt::make_bool(bool value)
{
*this=exprt(ID_constant, typet(ID_bool));
set(ID_value, value?ID_true:ID_false);
}

/// Replace the expression by a Boolean expression representing true.
/// \deprecated use constructors instead
void exprt::make_true()
{
*this=exprt(ID_constant, typet(ID_bool));
set(ID_value, ID_true);
}

/// Replace the expression by a Boolean expression representing false.
/// \deprecated use constructors instead
void exprt::make_false()
{
*this=exprt(ID_constant, typet(ID_bool));
set(ID_value, ID_false);
}

/// Return whether the expression represents a Boolean.
/// \return True if is a Boolean, false otherwise.
bool exprt::is_boolean() const
{
return type().id()==ID_bool;
}

/// Return whether the expression is a constant representing 0.
/// Will consider the following types: ID_integer, ID_natural, ID_rational,
/// ID_unsignedbv, ID_signedbv, ID_c_bool, ID_c_bit_field, ID_fixedbv,
/// ID_floatbv, ID_pointer.<br>
/// For ID_pointer, returns true iff the value is a zero string or a null
/// pointer.
/// For everything not in the above list, return false.
/// \return True if has value 0, false otherwise.
bool exprt::is_zero() const
{
if(is_constant())
Expand Down Expand Up @@ -202,6 +255,12 @@ bool exprt::is_zero() const
return false;
}

/// Return whether the expression is a constant representing 1.
/// Will consider the following types: ID_integer, ID_natural, ID_rational,
/// ID_unsignedbv, ID_signedbv, ID_c_bool, ID_c_bit_field, ID_fixedbv,
/// ID_floatbv.<br>
/// For all other types, return false.
/// \return True if has value 1, false otherwise.
bool exprt::is_one() const
{
if(is_constant())
Expand Down Expand Up @@ -243,6 +302,11 @@ bool exprt::is_one() const
return false;
}

/// Get a \ref source_locationt from the expression or from its operands
/// (non-recursively).
/// If no source location is found, a nil `source_locationt` is returned.
/// \return A source location if found in the expression or its operands, nil
/// otherwise.
const source_locationt &exprt::find_source_location() const
{
const source_locationt &l=source_location();
Expand Down
21 changes: 14 additions & 7 deletions src/util/expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,17 @@ class depth_iteratort;
class const_depth_iteratort;
class const_unique_depth_iteratort;

/*! \brief Base class for all expressions
*/
/// Base class for all expressions.
/// Inherits from \ref irept and has operands (stored as unnamed children of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Could be a bit newbie-friendlier, e.g. "Inherits from the tree structure irept" or something similar. We don't want to re-explain irept every time it's mentioned, but in it's direct subclasses I think a little bit of duplication won't do any harm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't know what irept is, I'm not sure it helps to mention that it's a tree structure. I'd rather the newbie clicked on the link! :-)

/// `irept`), and a type (which is a named sub with identifier `ID_type`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ In util/README.md the style seems to be to always use links (i.e. \ref irept) rather than first a link and then just monospace. Not sure if that's a standard though.

/// The class contains functions to access and modify the operands, as well as
/// visitor utilities.
///
/// The example below shows the irept structure of the sum of integers
/// 3 and 5.
/// \dotfile expr.dot
/// Some context available here: \ref exprt_section
/// "exprt section in util module"
class exprt:public irept
{
public:
Expand All @@ -52,14 +61,14 @@ class exprt:public irept
add(ID_type, _type);
}

// returns the type of the expression
/// Return the type of the expression
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: full stop at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we usually put a full stop at the end of the return statements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine with me, but you did that below

typet &type() { return static_cast<typet &>(add(ID_type)); }
const typet &type() const
{
return static_cast<const typet &>(find(ID_type));
}

// returns true if there is at least one operand
/// Return true if there is at least one operand.
bool has_operands() const
{ return !operands().empty(); }

Expand Down Expand Up @@ -96,16 +105,14 @@ class exprt:public irept
void reserve_operands(operandst::size_type n)
{ operands().reserve(n) ; }

// destroys expr, e1, e2, e3
void move_to_operands(exprt &expr);
void move_to_operands(exprt &e1, exprt &e2);
void move_to_operands(exprt &e1, exprt &e2, exprt &e3);
// does not destroy expr, e1, e2, e3

void copy_to_operands(const exprt &expr);
void copy_to_operands(const exprt &e1, const exprt &e2);
void copy_to_operands(const exprt &e1, const exprt &e2, const exprt &e3);

// the following are deprecated -- use constructors instead
void make_typecast(const typet &_type);
void make_not();

Expand Down