Add Doxygen comments for the TableKeyExtraction pass. #528

cole-barefoot · 2017-04-26T06:28:03Z

No description provided.

mihaibudiu · 2017-04-26T16:36:43Z

frontends/p4/tableKeyNames.cpp

@@ -32,10 +38,16 @@ class KeyNameGenerator : public Inspector {

    void postorder(const IR::Expression* expression) override { error(expression); }

+    /** Compute a name annotation for @expression.  Eg. `@name("foo.bar")` for


These comments on the postorder methods are practically useless.
This one in particular is also wrong, since a PathExpression cannot be "foo.bar".
The code of these methods is so short that it is self-explanatory; I don't think that these comments help at all.
What should be documented is the "name" field of the KeyNameGenerator; for each expression is maintains a cstring representing the tentative name of the expression. The name of an expression is built recursively from its components. if an expression does not have a name in this map, then it is too complex.

To me, the fact that some of these comments are not accurate suggests that we need to either simplify the code, or add documentation so anyone can easily understand it.

I think having Doxygen comments for most methods is reasonable for uniformity, even if the meaning is clear from the name and type signature.

Finally, if a PhD-level engineer stays up past midnight to add documentation to code that somebody else wrote, it seems only polite to avoid characterizing their effort as "practically useless" ;-)

I don't imply that the work is useless, that particular comment is useless. That is different.

Also, there is such a thing as over-commenting; we have no tools to keep comments in sync with code, and a wrong comment is worse than no comment. So I think we should really write comments where they really help our understanding of the code/algorithms. Sometimes code is clearer than comments.

I don't think that uniform documentation for Doxygen is a worthwhile goal.

Sorry, I read "These comments" more broadly and misunderstood.

I agree that too many comments can be overkill. I would be in favor of class comments that document the assumptions / guarantees of each pass and the overall strategy used to implement it. Members of the class and helper functions may also merit comments but we should use our judgment. I agree that commenting each pre/post-order method may be overkill if it's just implementing the functionality specified in the class comment, unless there is something exceptional going on in that method.

For this pass, I'd like to see examples of the names generated from the IR. This is much faster than, say, digging through the parser to figure out how syntax corresponds to IR objects.

For example, _ is apparently a valid PathExpression, but---looking at the parser---will not appear in an expression inside a KeyElement. (In fact, it's not obvious to me why the Path class has an isDontCare() method.) On the other hand, dot-prefixed identifiers are also valid PathExpressions and may indeed appear here, in which case the name generated includes the "." prefix.

This is not obvious from the implementation of the postorder method.

I don't have a strong preference as to whether this lives as part of comments on pre-/postorder methods, on the name field of the KeyNameGenerator, or as part of the class comment for the pass itself.

mihaibudiu · 2017-04-26T16:37:54Z

frontends/p4/tableKeyNames.cpp

@@ -57,6 +72,9 @@ class KeyNameGenerator : public Inspector {
        name.emplace(expression, l + "[" + r + "]");
    }

+    /** Compute a name annotation for @expression.  Eg. `@name("bits")` for
+     * `bits & 0x3` or `0x3 & bits`.


Note that this method is somewhat of a hack designed to support masks for keys, which are a P4-14 feature only.
I can argue that this should not be done here, but in the P4-14 front-end.

Why do you say it is a P4-14 feature only? The current p4c-bm2-ss compiler takes 'field_name & 0xff00 : exact;' as a key in a table with no error, and uses the mask feature in the bmv2 JSON file to represent it. You can even put other expressions there like "field_name | 0xff00 : exact @name("foo");", but in that case the P4-16 compiler creates a temporary variable to hold the result of the expression, and then sends that as the table search key.

The P4-16 draft spec allows both of these things in the syntax. I'm not here making an argument that these things must be kept in the spec, but they are currently there and should be removed if you think support for it should go away.

Key masks are a P4-14 feature only; it is not clear that they are semantically the same as using & on a key. In particular, we don't know how the control-plane specifies such keys.

The name annotation should be inserted by the P4-14 front-end when converting a key mask.
Otherwise we should not handle this specially at all.

mihaibudiu · 2017-04-26T16:38:42Z

frontends/p4/tableKeyNames.cpp

@@ -68,10 +86,17 @@ class KeyNameGenerator : public Inspector {
            error(expression); }
    }

+    /** Compute a name annotation for @expression.  Eg. `@name("16w4")` for


I expect that the result would be 4 instead of 16w4.
I don't think that the control-plane (the one that consumes these annotations) supports widths.

mihaibudiu · 2017-04-26T16:40:18Z

frontends/p4/tableKeyNames.cpp

    void postorder(const IR::Constant* expression) override {
        name.emplace(expression, expression->toString());
    }

+
+    /** Compute a name annotation for @expression.  Eg. `@name("foo[0:3]")` for


The slice syntax is really MSB:LSB.

mihaibudiu · 2017-04-26T16:42:56Z

frontends/p4/tableKeyNames.cpp

+    /** Compute a name annotation if @expression is a method call for
+     * `isValid()`.
+     *
+     * @warning All other method calls in keys will cause the compiler to


The comment is inaccurate; the compiler will not reject other method calls; this method will invoke an error if it is invoked with another method call. However, if the key already has a name annotation, the compiler will not reject the program.

mihaibudiu · 2017-04-26T16:44:11Z

frontends/p4/tableKeyNames.h

@@ -23,13 +23,6 @@ limitations under the License.

 namespace P4 {

-/**


Technically this comment is correct, this is the pass that adds the annotations.
The PassManager below just calls this one.

mihaibudiu · 2017-04-26T16:44:38Z

frontends/p4/tableKeyNames.h

+ * compiler will give an error.  Simple expressions are:
+ * - .isValid(),
+ * - ArrayIndex,
+ * - BAnd (where at least one operand is constant),


I think that the second operand must be constant.
And I think that this case does not belong here.

Makes sense. I'll open a low-priority issue.

mihaibudiu · 2017-04-26T16:46:03Z

frontends/p4/tableKeyNames.h

+ * The string used for the name is derived from the expression itself - if
+ * the expression is "simple" enough.  If the expression is not simple the
+ * compiler will give an error.  Simple expressions are:
+ * - .isValid(),


There is an open issue #366 about handling isValid(). I don't think that is correct either, but we don't know what else to use until we have a spec for the control-plane API.

mihaibudiu · 2017-04-26T16:46:18Z

frontends/p4/tableKeyNames.h

+ * - Slice.
+ *
+ * @pre None.
+ * @post All keys have `@name` annotations.


All key fields have annotations.

cole-barefoot · 2017-04-28T00:39:07Z

I consolidated examples in the class comment rather than with each postorder method. This, coupled with better documentation of the IR, should be clear.

cole-barefoot requested a review from mihaibudiu April 26, 2017 06:29

mihaibudiu requested changes Apr 26, 2017

View reviewed changes

cole-barefoot force-pushed the document_passes branch from 1547e7e to 707f1de Compare April 28, 2017 00:38

cole-barefoot mentioned this pull request Apr 28, 2017

P4_14 frontend should insert name annotations for bitwise-and key expressions. #539

Closed

ChrisDodd approved these changes May 1, 2017

View reviewed changes

mihaibudiu approved these changes May 1, 2017

View reviewed changes

cole-barefoot force-pushed the document_passes branch from 707f1de to d30c501 Compare May 1, 2017 22:27

cole-barefoot added 3 commits May 1, 2017 16:53

Add Doxygen comments for tableKeyNames pass.

e02bd58

Revision based on feedback from @mbudiu-vmw.

6f3edb6

Move examples from method comments to class comment.

8bc2bc5

cole-barefoot force-pushed the document_passes branch from d30c501 to 8bc2bc5 Compare May 1, 2017 23:53

cole-barefoot merged commit 0c06bcb into p4lang:master May 2, 2017

cole-barefoot deleted the document_passes branch May 2, 2017 00:25

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

Add Doxygen comments for the TableKeyExtraction pass. #528

Add Doxygen comments for the TableKeyExtraction pass. #528

cole-barefoot commented Apr 26, 2017

mihaibudiu Apr 26, 2017

jnfoster Apr 26, 2017

mihaibudiu Apr 26, 2017

jnfoster Apr 26, 2017

cole-barefoot Apr 26, 2017

mihaibudiu Apr 26, 2017

jafingerhut Apr 26, 2017 •

edited

Loading

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

cole-barefoot Apr 28, 2017

mihaibudiu Apr 26, 2017

mihaibudiu Apr 26, 2017

cole-barefoot commented Apr 28, 2017

		@@ -32,10 +38,16 @@ class KeyNameGenerator : public Inspector {

		void postorder(const IR::Expression* expression) override { error(expression); }

		/** Compute a name annotation for @expression. Eg. `@name("foo.bar")` for

		@@ -23,13 +23,6 @@ limitations under the License.

		namespace P4 {

		/**

Add Doxygen comments for the TableKeyExtraction pass. #528

Add Doxygen comments for the TableKeyExtraction pass. #528

Conversation

cole-barefoot commented Apr 26, 2017

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

jafingerhut Apr 26, 2017 • edited Loading

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

cole-barefoot commented Apr 28, 2017

jafingerhut Apr 26, 2017 •

edited

Loading