Doxygen comments #614

hanw · 2017-05-11T14:36:48Z

Comment improvements for these four passes:

simplifySelectCases
simplifyKey
removeLeftSlices
removeActionParameters

jnfoster

Overall, looks great. Just some minor typesetting and phrasing suggestions.

Also, it'd be nice to have @pre/@post conditions for the passes.

jnfoster · 2017-05-11T15:25:27Z

midend/removeLeftSlices.h

+/**
+ * This pass removes Slices on the lhs of an assignment
+ *
+ * a[m:l] = e;  ->  a = (a & ~mask) | (((cast)e << l) & mask);


Nice! The example illustrates this transformation perfectly.

Will this be typeset as code in Doxygen?

jnfoster · 2017-05-11T15:25:49Z

midend/removeParameters.cpp

-
+/**
+ * This is needed because of this case:
+ * action a(inout x) { x = x + 1 }


Will this be typeset as code in Doxygen?

jnfoster · 2017-05-11T15:26:29Z

midend/removeParameters.h

+/**
+ * For each action that is invoked keep the list of arguments that
+ * it's called with. There must be only one call of each action;
+ * this is done by LocalizeActions.


Would be nice to use @pre and @post

jnfoster · 2017-05-11T15:26:42Z

midend/removeParameters.h

+/**
+Removes parameters of an action which are in/inout/out.
+
+control c(inout bit<32> x) {


Doxygen formatting for code?

jnfoster · 2017-05-11T15:27:54Z

midend/simplifyKey.h

-// Policy which returns 'true' whenever a key is not a left value
-// or a call to the isValid().
+/**
+ * Policy which returns 'true' whenever a key is not a left value


Instead of saying "returns" how about a different verb? For example "Policy that tracks whether a key is..."

jnfoster · 2017-05-11T15:28:12Z

midend/simplifyKey.h

@@ -42,7 +46,9 @@ class NonLeftValue : public KeyIsComplex {
    bool isTooComplex(const IR::Expression* expression) const;


How does "too complex" related to l-value-ness?

The pass applies to non-lvalue expressions. All lvalues are considered not complex and therefore not transformed.

jnfoster · 2017-05-11T15:28:35Z

midend/simplifyKey.h

@@ -42,7 +46,9 @@ class NonLeftValue : public KeyIsComplex {
    bool isTooComplex(const IR::Expression* expression) const;
 };

-// Policy that allows masked lvalues as well as simple lvalues or isValid()
+/**
+ * Policy that allows masked lvalues as well as simple lvalues or isValid()


Again, reword as something like "Policy that tracks whether an lvalue is masked, simple, ..."

jnfoster · 2017-05-11T15:28:51Z

midend/simplifyKey.h

-// If some of the fields of a table key are "too complex",
-// turn them into simpler expressions.
+/**
+ * If some of the fields of a table key are "too complex",


"Transforms complex table keys into..."

jnfoster · 2017-05-11T15:29:19Z

midend/simplifyKey.h

@@ -86,7 +116,9 @@ class DoSimplifyKey : public Transform {
    const IR::Node* postorder(IR::P4Table* table) override;
 };

-// Use a policy to decide when a key expression is too complex.
+/**
+ * This pass uses a policy to decide when a key expression is too complex.


"Determine whether a key expression is too complex"

Also, "too complex" should be defined above.

mihaibudiu · 2017-05-11T20:02:59Z

midend/removeParameters.cpp

+/**
+ * This is needed because of this case:
+ *
+ * \code{.cpp}


This comment is in the wrong place, it was explaining why MoveDeclarations is required. This is not what this pass is doing. The comment for the pass is in the header file. This is a code comment.

mihaibudiu · 2017-05-11T20:04:00Z

midend/removeParameters.h

@@ -45,10 +48,10 @@ class ActionInvocation {
    }
    void bindDefaultAction(const IR::P4Action* action,
                           const IR::MethodCallExpression* defaultInvocation) {
-        // We must have a binding for this action already.
+        /// We must have a binding for this action already.


These should not be Doxygen comments, they are commenting code, not APIs.

mihaibudiu · 2017-05-11T20:05:10Z

midend/removeParameters.h

-//    table t() { actions = { a; } }
-//    apply { ... } }
+/**
+Removes parameters of an action which are in/inout/out.


Your commenting style is inconsistent, sometimes you use a star, sometimes you just indent, and sometimes you don't indent.

mihaibudiu · 2017-05-11T20:05:54Z

midend/removeParameters.h

    const IR::Node* postorder(IR::ActionListElement* element) override;
+    /// Remove arguments from method call


I think that these comments are unnecessary.
Moreover, this only does this for method calls that are calling actions, to the comment is inaccurate.

mihaibudiu · 2017-05-11T20:06:14Z

midend/simplifyKey.h

-// Policy used to decide whether a key expression is too complex.
+/**
+ * Policy used to decide whether a key expression is too complex.
+ */
 class KeyIsComplex {
 public:
    virtual ~KeyIsComplex() {}
    virtual bool isTooComplex(const IR::Expression* expression) const = 0;


The isTooComplex function requires a comment.

mihaibudiu · 2017-05-11T20:08:06Z

midend/simplifyKey.h

@@ -86,7 +120,13 @@ class DoSimplifyKey : public Transform {
    const IR::Node* postorder(IR::P4Table* table) override;
 };

-// Use a policy to decide when a key expression is too complex.
+/**


This is incorrect, this pass actually simplifies the keys, it does not determine anything.
You deleted the original comment, which indicated what the policy parameter is about; that should become a @param comment.

mihaibudiu · 2017-05-11T20:09:19Z

midend/simplifySelectCases.h

+ * If requireConstants is true this pass requires that
+ * all select labels evaluate to constants.
+ *
+ * @pre None


Well, at least the typeMap must be correct. But maybe that is implied.
This pass assumes that there are no side-effects in the evaluation of select labels, something which is not yet enforced by the compiler or the spec.

mihaibudiu · 2017-05-11T20:09:44Z

midend/validateProperties.h

@@ -36,7 +41,7 @@ class ValidateTableProperties : public Inspector {
            legalProperties.emplace(l);
    }
    void postorder(const IR::Property* property) override;
-    /* don't check properties in externs (Declaration_Instances) */
+    /// don't check properties in externs (Declaration_Instances)


This should probably not be a Doxygen comment.

mihaibudiu · 2017-05-11T20:10:11Z

midend/validateProperties.h

+ * Checks to see if there are any unknown properties.
+ *
+ * @pre none
+ * @post no invalid table properties in P4 program.


Actually the properties are not removed, but an error is given if such properties are found.

mihaibudiu · 2017-05-11T20:34:07Z

midend/simplifyKey.h

- *
- * Any non-lvalues, except built-in method invocation (isValid),
- * are considered complex expressions.
+ *  This pass considers any non-lvalues, except built-in method invocation (isValid),


This is incorrect, whether a key is complex or not is determined by the policy.
This pass does not know whether non l-values are complex or not.

mihaibudiu · 2017-05-11T20:54:11Z

midend/simplifyKey.h

- *
- * @param policy Use 'policy' to determine whether a key expression is too complex.
+ * This pass uses 'policy' to determine whether a key expression is too complex.
+ * Policies are defined in 'NonLeftValue' and 'NonMaskLeftValue', which consider


This pass should not list all possible policies; we can't be updating this comment every time we write a new policy.
Each policy has its own comments. This pass also simplifies keys which are complex according to the policy.

mihaibudiu · 2017-05-11T21:00:01Z

midend/simplifyKey.h

- * Policies are defined in 'NonLeftValue' and 'NonMaskLeftValue', which consider
- * any non-lvalue, except built-in method (isValid) in table key expressions
- * as complex expressions.
+ * and simplifies keys which are complex according to the policy.


I think you also need to remove the do after 'complex'

Han Wang and others added 7 commits May 11, 2017 05:48

add comments on removeActionParameters pass

cbb2329

reformat comment in removeLeftSlices

7c18e91

add comments in simplifyKey

d33e67f

reformat comments in simplifySelectCases pass

deb9e6a

doxygen comment for validateProperties

5f59a6d

doxygen comment in moveConstructors

e949988

Merge branch 'master' into code-review

77963bd

jnfoster requested changes May 11, 2017

View reviewed changes

Han Wang added 3 commits May 11, 2017 09:54

update with nate's comments

9e90674

add pre and post conditions in passes

38b609e

revert change to bootstrap.sh

1009b53

mihaibudiu requested changes May 11, 2017

View reviewed changes

address Mihai's comments

005edb1

mihaibudiu requested changes May 11, 2017

View reviewed changes

improve comments in simplifyKey.h

4033dc0

mihaibudiu requested changes May 11, 2017

View reviewed changes

correct comments in simplifyKeys

9e5d5bd

mihaibudiu approved these changes May 11, 2017

View reviewed changes

Han Wang and others added 2 commits May 11, 2017 14:03

minor fix

7de9191

Merge branch 'master' into code-review

bc99186

jnfoster approved these changes May 11, 2017

View reviewed changes

hanw merged commit 58b7136 into p4lang:master May 11, 2017

hanw deleted the code-review branch May 11, 2017 22:14

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doxygen comments #614

Doxygen comments #614

hanw commented May 11, 2017

jnfoster left a comment

jnfoster May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

hanw May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

hanw May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

jnfoster May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

mihaibudiu May 11, 2017

		@@ -42,7 +46,9 @@ class NonLeftValue : public KeyIsComplex {
		bool isTooComplex(const IR::Expression* expression) const;

		const IR::Node* postorder(IR::ActionListElement* element) override;
		/// Remove arguments from method call

Doxygen comments #614

Doxygen comments #614

Conversation

hanw commented May 11, 2017

jnfoster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment