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

cleanup: Modernize C++ struct/enum/union declarations #1588

Conversation

federico-sysdig
Copy link
Contributor

What type of PR is this?
/kind cleanup

Any specific area of the project related to this PR?

Does this PR require a change in the driver versions?
no

What this PR does / why we need it:
From the falcosecurity/libs project's coding guidelines:

Also we think that defining the struct as a typedef makes forward declarations
clunky and find using the C++ style when declaring our structs makes our
lives easier.

   //
   // Us human parsers find this confusing.
   //
   typedef struct _my_struct
   {
     u_int16	m_field;
   } my_struct,
   *p_my_struct;

   //
   // This is easier!
   //
   struct my_struct {
     u_int16	m_field;
   };

This PR changes struct/union/enum definitions to follow just that.
Also, it changes declarations of objects of struct/union/enum type as to remove the struct/union/enum in the C++ code as user-defined types are "first-class citizens" in C++ and don't need a special, longer, syntax.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
The changes are very many, but consider that they are all instances of the same change. For instance struct sinsp_chisel* ch; becomes sinsp_chisel* ch;. Apart the mechanical change, there is no difficult logic to be reviewed in this PR.

Does this PR introduce a user-facing change?:

NONE

@incertum
Copy link
Contributor

Same, could we get a rebase, thanks!

@federico-sysdig federico-sysdig force-pushed the cleanup-modernize-struct-union-enum branch from 7a75825 to c76d944 Compare January 16, 2024 20:37
@federico-sysdig
Copy link
Contributor Author

Same, could we get a rebase, thanks!

@incertum Thanks for the nudge. Done.

@federico-sysdig federico-sysdig force-pushed the cleanup-modernize-struct-union-enum branch from c76d944 to d3a5d92 Compare January 17, 2024 11:31
incertum
incertum previously approved these changes Jan 18, 2024
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jan 18, 2024

LGTM label has been added.

Git tree hash: f2ce7af701eaaa3883a23a844e7293bc31643960

jasondellaluce
jasondellaluce previously approved these changes Jan 19, 2024
Signed-off-by: Federico Aponte <federico.aponte@sysdig.com>
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Jan 20, 2024
@poiana
Copy link
Contributor

poiana commented Jan 20, 2024

LGTM label has been added.

Git tree hash: f2a0e4a125439bb5ee2fe0fbf7bd2b33ae83586c

Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jan 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, federico-sysdig, incertum

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit b13c5f9 into falcosecurity:master Jan 22, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants