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.

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

doc: backends: Add Doxygen-style documentation for BMV2 JSONObjects #4554

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

Djain318
Copy link
Contributor

  1. Class Overview:

    • Provides an overview of the JsonObjects class, explaining its purpose and namespace.
  2. Constructor:

    • Documented the constructor JsonObjects::JsonObjects() with a brief description of its purpose and initialization of member variables.
  3. Member Functions:

    • Each member function of the JsonObjects class is documented with detailed descriptions.

    • Special attention is given to functions with complex logic or multiple parameters to ensure detailed documentation.

Overall, this commit enhances the understanding of the BMV2 JSON Objects class by providing a Doxygen-style documentation.

@Djain318 Djain318 force-pushed the doc/backends_documentation branch 4 times, most recently from 271c2e6 to 4fc6672 Compare March 22, 2024 11:46
@Djain318
Copy link
Contributor Author

Hello sir, @Dscano @fruffy
Some checks are not passing in this PR, and I'm not getting the reason why cmake is failing.

I only made comments in this file which should not hinder the process. But then too checks are failing.

Please help

@fruffy
Copy link
Collaborator

fruffy commented Mar 23, 2024

Yes, we have linters which enforce code style. Please take a look at the failing checks and what they report. In this case the error is:
/home/runner/work/p4c/p4c/backends/bmv2/common/JsonObjects.h:49: Should have a space between // and comment [whitespace/comments] [4]

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 23, 2024

@fruffy @Dscano sir,
Thanks

Could you please let me know how I can see these errors at my end??

@Djain318 Djain318 force-pushed the doc/backends_documentation branch 3 times, most recently from 80e1549 to e9bdb7d Compare March 23, 2024 03:10
@fruffy fruffy added the documentation Topics related to compiler documentation. label Mar 24, 2024
@fruffy
Copy link
Collaborator

fruffy commented Mar 24, 2024

These comments look good, but they may be excessive. We could just use simple three slash comments describing each function. To be honest we have never defined a consistent commenting style for the compiler. Ideally, we would like these comments to be consistent across the compiler.

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 25, 2024

Hello sir, @fruffy
we can surely comment any part by using /// but it will only gives one liner discription, and might be difficult to understand the purpose of any member function sometime.

Here is the output snippet of documentation site for the detailed commenting I made in this PR.

Screenshot (139)

Also, I'm open to make any changes.
Please provide instructions for further enhancements, so that I can do the documentation in same manner for other files too.

@fruffy
Copy link
Collaborator

fruffy commented Mar 25, 2024

we can surely comment any part by using /// but it will only gives one liner discription, and might be difficult to understand the purpose of any member function sometime.

Are you sure about /// only supporting one liners? I believe they should be equivalent. The reason I am asking is because a lot of code in the compiler uses triple slashes.

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 25, 2024

Yes sir, @fruffy
As we know that we can comment any line in c++ by//. So, doxygen uses the same convention to parse comments by adding one more /

If we want to use // as multiline comment we have to put it in everyline we need to comment.
/// line 1
/// line 2
/// line 3

But if we want to comment to a block of code together, we use
/*
Line1
Line2
*/

That mean, there are 2 commenting styles in c++ for different purpose.

@fruffy
Copy link
Collaborator

fruffy commented Mar 26, 2024

Yes sir, @fruffy As we know that we can comment any line in c++ by//. So, doxygen uses the same convention to parse comments by adding one more /

If we want to use // as multiline comment we have to put it in everyline we need to comment. /// line 1 /// line 2 /// line 3

But if we want to comment to a block of code together, we use /* Line1 Line2 */

That mean, there are 2 commenting styles in c++ for different purpose.

Yes, ideally we will use the first commenting style /// across the compiler. It should work fine with doxygen, no?

Although this is still something that needs to be discussed with the rest of the maintainers.

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 26, 2024

Yes, ideally we will use the first commenting style /// across the compiler. It should work fine with doxygen, no?

Absolutely, /// also works fine.

I think both styles were used as per the requirement.
You can refer here at JsonObjects for detailed comments and on same file here for brief (single liners)

@fruffy
Copy link
Collaborator

fruffy commented Mar 26, 2024

I think there is a miscommunication going on, the idea is to never use /**/ and only use ///. Like this code here

/// Serialize this control-plane API to the provided output stream, using

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 26, 2024

Sure sir @fruffy,
So, can I finally change the comments of this PR to the form you finalized i.e /// and proceed further with other files too ??

@Djain318 Djain318 force-pushed the doc/backends_documentation branch 2 times, most recently from 85221b8 to 1509937 Compare March 28, 2024 15:12
@Djain318
Copy link
Contributor Author

Hello @fruffy,
I made the changes as per the discussion #4563

Please review and suggest changes.

Thanks

@AdarshRawat1
Copy link
Member

@thrilseekr Before you can merge your PR, you must sign the Contributor Licence Agreement (CLA) and add your GitHub user ID to it.

Link to CLA - https://cla.opennetworking.org/

@Djain318
Copy link
Contributor Author

Djain318 commented Mar 28, 2024

@thrilseekr Before you can merge your PR, you must sign the Contributor Licence Agreement (CLA) and add your GitHub user ID to it.

Link to CLA - https://cla.opennetworking.org/

Thanks @AdarshRawat1
I forgot to did so.
Now signedin :)

@Djain318 Djain318 force-pushed the doc/backends_documentation branch from 1509937 to 573fe00 Compare March 28, 2024 17:44
1. Class Overview:
   - Provides an overview of the JsonObjects class, explaining its purpose and namespace.

2. Constructor:
   - Documented the constructor `JsonObjects::JsonObjects()` with a brief description of its purpose and initialization of member variables.

3. Member Functions:
   - Each member function of the JsonObjects class is documented with detailed descriptions.

   - Special attention is given to functions with complex logic or multiple parameters to ensure detailed documentation.

Overall, this commit enhances the understanding of the BMV2 JSON Objects class by providing a Doxygen-style documentation.
@Djain318 Djain318 force-pushed the doc/backends_documentation branch from 573fe00 to 4170ec9 Compare March 28, 2024 17:50
Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Moving the documentation to the header file is a good clean-up. Thanks!

I am a little torn whether we need to have @param @brief @return etc because they might simply be too excessive. But that we need to discuss separately.

@fruffy fruffy added this pull request to the merge queue Apr 1, 2024
Merged via the queue into p4lang:main with commit 27eaccf Apr 1, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Topics related to compiler documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants