Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Consistent Commenting Style in backend #4568

Closed
wants to merge 4 commits into from
Closed

Implement Consistent Commenting Style in backend #4568

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10b6239
Corrected version in README.md
sumit7754 Mar 24, 2024
7200638
Refactored backend comments for consistency
sumit7754 Mar 27, 2024
0561da8
Merge branch 'main' into update-readme
sumit7754 Mar 27, 2024
6d47e2c
Merge branch 'p4lang:main' into update-readme
sumit7754 Mar 29, 2024
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
43 changes: 21 additions & 22 deletions backends/bmv2/common/JsonObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
/*
Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
/// Copyright 2013-present Barefoot Networks, Inc.
///
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at
///
/// http://www.apache.org/licenses/LICENSE-2.0
///
/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

#include "JsonObjects.h"

Expand Down Expand Up @@ -105,14 +104,14 @@ void JsonObjects::add_meta_info() {
toplevel->emplace("__meta__", meta);
}

/**
* Create a header type in json.
* @param name header name
* @param type header type
* @param max_length maximum length for a header with varbit fields;
* if 0 header does not contain varbit fields
* @param fields a JsonArray for the fields in the header
*/

/// Create a header type in json.
/// @param name header name
/// @param type header type
/// @param max_length maximum length for a header with varbit fields;
/// if 0 header does not contain varbit fields
/// @param fields a JsonArray for the fields in the header

unsigned JsonObjects::add_header_type(const cstring &name, Util::JsonArray *&fields,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You added an additional space here between the comment and the C++ code. Please remove it for all instances.

unsigned max_length) {
std::string sname(name, name.size());
Expand Down
25 changes: 12 additions & 13 deletions backends/bmv2/common/action.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
/*
Copyright 2013-present Barefoot Networks, Inc.
/// Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0
/// http://www.apache.org/licenses/LICENSE-2.0

/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#include "action.h"

Expand Down Expand Up @@ -213,4 +212,4 @@ void ActionConverter::postorder(const IR::P4Action *action) {
ctxt->structure->ids.emplace(action, id);
}

} // namespace BMV2
} /// namespace BMV2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the namespace should I use /// or // ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

They are internal, use two slashes here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, another helpful contribution is to write this all down. Very good to have! We should have done this much earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure we can do this.

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 elaborate more then that will be helpful

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically, all these details we have discussed (comment style for namespaces, triple slashes vs star comments, newlines before functions) should be written down in text. This way we can refer to our "style document" whenever these questions come up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* Convert an expression into JSON
* @param e expression to convert
* @param doFixup Insert masking operations for operands to ensure that the result
* matches the specification. BMv2 does arithmetic using unbounded
* precision, but the spec requires fixed precision, specified by the types.
* @param wrap Wrap the result into an additiona JSON expression block.
* See the BMv2 JSON spec.
* @param convertBool Wrap the result into a cast from boolean to data (b2d JSON).
*/ For this type of comments, what kind of formatting should we use?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Triple slashes.

30 changes: 14 additions & 16 deletions backends/bmv2/common/action.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
/*
Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
/// Copyright 2013-present Barefoot Networks, Inc.
///
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at
///
/// http://www.apache.org/licenses/LICENSE-2.0
///
/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

#ifndef BACKENDS_BMV2_COMMON_ACTION_H_
#define BACKENDS_BMV2_COMMON_ACTION_H_
Expand Down Expand Up @@ -40,4 +38,4 @@ class ActionConverter : public Inspector {

} // namespace BMV2

#endif /* BACKENDS_BMV2_COMMON_ACTION_H_ */
#endif // BACKENDS_BMV2_COMMON_ACTION_H_
34 changes: 16 additions & 18 deletions backends/bmv2/common/annotations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
/*
Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
/// Copyright 2013-present Barefoot Networks, Inc.
///
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at
///
/// http://www.apache.org/licenses/LICENSE-2.0
///
/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

#ifndef BACKENDS_BMV2_COMMON_ANNOTATIONS_H_
#define BACKENDS_BMV2_COMMON_ANNOTATIONS_H_
Expand All @@ -22,9 +20,9 @@ limitations under the License.

namespace BMV2 {

/*
* Parses BMV2-specific annotations.
*/

/// Parses BMV2-specific annotations.

class ParseAnnotations : public P4::ParseAnnotations {
public:
ParseAnnotations()
Expand Down
66 changes: 33 additions & 33 deletions backends/bmv2/common/backend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
/*
Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
// Copyright 2013-present Barefoot Networks, Inc.

http://www.apache.org/licenses/LICENSE-2.0
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
// http://www.apache.org/licenses/LICENSE-2.0

// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.


#ifndef BACKENDS_BMV2_COMMON_BACKEND_H_
#define BACKENDS_BMV2_COMMON_BACKEND_H_
Expand Down Expand Up @@ -55,7 +55,7 @@ enum block_t {

class ExpressionConverter;

// Backend is a the base class for SimpleSwitchBackend and PortableSwitchBackend.
/// Backend is a the base class for SimpleSwitchBackend and PortableSwitchBackend.
class Backend {
public:
BMV2Options &options;
Expand All @@ -81,13 +81,13 @@ class Backend {
virtual void convert(const IR::ToplevelBlock *block) = 0;
};

/**
This class implements a policy suitable for the SynthesizeActions pass.
The policy is: do not synthesize actions for the controls whose names
are in the specified set.
For example, we expect that the code in the deparser will not use any
tables or actions.
*/

///This class implements a policy suitable for the SynthesizeActions pass.
///The policy is: do not synthesize actions for the controls whose names
///are in the specified set.
///For example, we expect that the code in the deparser will not use any
///tables or actions.

class SkipControls : public P4::ActionSynthesisPolicy {
// set of controls where actions are not synthesized
const std::set<cstring> *skip;
Expand All @@ -100,13 +100,13 @@ class SkipControls : public P4::ActionSynthesisPolicy {
}
};

/**
This class implements a policy suitable for the RemoveComplexExpression pass.
The policy is: only remove complex expression for the controls whose names
are in the specified set.
For example, we expect that the code in ingress and egress will have complex
expression removed.
*/

/// This class implements a policy suitable for the RemoveComplexExpression pass.
/// The policy is: only remove complex expression for the controls whose names
/// are in the specified set.
/// For example, we expect that the code in ingress and egress will have complex
/// expression removed.

class ProcessControls : public P4::RemoveComplexExpressionsPolicy {
const std::set<cstring> *process;

Expand All @@ -120,12 +120,12 @@ class ProcessControls : public P4::RemoveComplexExpressionsPolicy {
}
};

/**
This pass adds @name annotations to all fields of the user metadata
structure so that they do not clash with fields of the headers
structure. This is necessary because both of them become global
objects in the output json.
*/

/// This pass adds @name annotations to all fields of the user metadata
/// structure so that they do not clash with fields of the headers
/// structure. This is necessary because both of them become global
/// objects in the output json.

class RenameUserMetadata : public Transform {
P4::ReferenceMap *refMap;
const IR::Type_Struct *userMetaType;
Expand Down
33 changes: 16 additions & 17 deletions backends/bmv2/common/controlFlowGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
/*
Copyright 2013-present Barefoot Networks, Inc.
/// Copyright 2013-present Barefoot Networks, Inc.
///
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at
///
/// http://www.apache.org/licenses/LICENSE-2.0
///
/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

#include "controlFlowGraph.h"

Expand Down Expand Up @@ -121,8 +120,8 @@ bool CFG::EdgeSet::checkSame(const CFG::EdgeSet &other) const {
return true;
}

// We check whether a table always jumps to the same destination,
// even if it appears multiple times in the CFG.
/// We check whether a table always jumps to the same destination,
/// even if it appears multiple times in the CFG.
bool CFG::checkMergeable(std::set<TableNode *> nodes) const {
TableNode *first = nullptr;
for (auto tn : nodes) {
Expand Down Expand Up @@ -166,7 +165,7 @@ bool CFG::checkImplementable() const {
namespace {
class CFGBuilder : public Inspector {
CFG *cfg;
/// predecessors of current CFG node
// predecessors of current CFG node
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct. Members in header files also will have triple slashes. We only use double slashes for "internal" comments within functions, for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix this for all instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// Create a header type in json.
/// @param name header name
/// @param type header type
/// @param max_length maximum length for a header with varbit fields;
/// if 0 header does not contain varbit fields
/// @param fields a JsonArray for the fields in the header

Are you referring to a gap in these comments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can format Github comments using triple '`', it will make your comment more readable: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-and-highlighting-code-blocks

Yes, in all the comments where there is a newline between the comments and the actual function.

const CFG::EdgeSet *live;
P4::ReferenceMap *refMap;
P4::TypeMap *typeMap;
Expand Down
40 changes: 19 additions & 21 deletions backends/bmv2/common/controlFlowGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
/*
Copyright 2013-present Barefoot Networks, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
/// Copyright 2013-present Barefoot Networks, Inc.
///
/// Licensed under the Apache License, Version 2.0 (the "License");
/// you may not use this file except in compliance with the License.
/// You may obtain a copy of the License at
///
/// http://www.apache.org/licenses/LICENSE-2.0
///
/// Unless required by applicable law or agreed to in writing, software
/// distributed under the License is distributed on an "AS IS" BASIS,
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
/// See the License for the specific language governing permissions and
/// limitations under the License.

#ifndef BACKENDS_BMV2_COMMON_CONTROLFLOWGRAPH_H_
#define BACKENDS_BMV2_COMMON_CONTROLFLOWGRAPH_H_
Expand Down Expand Up @@ -115,18 +113,18 @@ class CFG final : public IHasDbPrint {
enum class EdgeType { Unconditional, True, False, Label };

public:
/**
* A CFG Edge; can be an in-edge or out-edge.
*/

/// A CFG Edge; can be an in-edge or out-edge.

class Edge final {
protected:
EdgeType type;
Edge(Node *node, EdgeType type, cstring label) : type(type), endpoint(node), label(label) {}

public:
/**
* The destination node of the edge. The source node is not known by the edge
*/

// The destination node of the edge. The source node is not known by the edge

Node *endpoint;
cstring label; // only present if type == Label

Expand Down
4 changes: 2 additions & 2 deletions backends/bmv2/common/expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,8 @@ bool ExpressionConverter::isArrayIndexRuntime(const IR::Expression *e) {
return false;
}

// doFixup = true -> insert masking operations for proper arithmetic implementation
// see below for wrap
/// doFixup = true -> insert masking operations for proper arithmetic implementation
/// see below for wrap
Util::IJson *ExpressionConverter::convert(const IR::Expression *e, bool doFixup, bool wrap,
bool convertBool) {
const IR::Expression *expr = e;
Expand Down
Loading
Loading