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

Removed a_ prefix on arguments #2513

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

JohanBertrand
Copy link
Contributor

@JohanBertrand JohanBertrand commented Feb 1, 2024

Related Issue(s) #2492
Has Unit Tests (y/n) yes
Documentation Included (y/n) yes

Change Description

Removed the prefix _a previously added in #2482

Changes:

  • Changed public variables of Fw/FilePacket and Svc/FileDownlink to PRIVATE variables

  • Changed public variable name to private variable m_name in STest/STest/Rule/Rule.hpp

  • Remove object initializion using public variable list in Svc/FileDownlink/FileDownlink.cpp and Fw/FilePacket/StartPacket.cpp

  • Added getters for variables new private variables
    The getters have been implemented in the header. Sometimes using references, sometimes copying values.
    If it is not standard, I am happy to update it.

  • Fixed UT

Rationale

The a_ prefix should not be used for arguments

Testing/Review Recommendations

I am not sure if there are some breaking changes for other projects

STest/STest/Rule/Rule.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

One question: @bocchino many functions where implemented in the header as opposed to the CPP. Does this match the C++ style we use?

@@ -19,17 +19,17 @@
namespace Fw {

void FilePacket::PathName ::
initialize(const char *const a_value)
initialize(const char *const value)

Check notice

Code scanning / CodeQL

Use of basic integral type Note

value uses the basic integral type char rather than a typedef with size and signedness.
const char *const a_sourcePath,
const char *const a_destinationPath
const U32 fileSize,
const char *const sourcePath,

Check notice

Code scanning / CodeQL

Use of basic integral type Note

sourcePath uses the basic integral type char rather than a typedef with size and signedness.
const char *const a_destinationPath
const U32 fileSize,
const char *const sourcePath,
const char *const destinationPath

Check notice

Code scanning / CodeQL

Use of basic integral type Note

destinationPath uses the basic integral type char rather than a typedef with size and signedness.
};

//! Get the path name value
const char* getValue(void) const {

Check notice

Code scanning / CodeQL

Use of basic integral type Note

getValue uses the basic integral type char rather than a typedef with size and signedness.

//! Pointer to the path value
const char *value;
const char *m_value;

Check notice

Code scanning / CodeQL

Use of basic integral type Note

m_value uses the basic integral type char rather than a typedef with size and signedness.
if (status != Os::File::OP_OK)
return status;

NATIVE_INT_TYPE intSize = a_size;
status = this->osFile.read(data, intSize);
NATIVE_INT_TYPE intSize = size;

Check notice

Code scanning / CodeQL

Use of basic integral type Note

intSize uses the basic integral type int rather than a typedef with size and signedness.
@@ -291,7 +354,7 @@
// Constructor
// ----------------------------------------------------------------------

FilePacket() { this->m_header.type = T_NONE; }
FilePacket() { this->m_header.m_type = T_NONE; }

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 3 statements; only one is allowed.
@@ -77,23 +77,21 @@
public:

//! Constructor
File() : size(0) { }
File() : m_size(0) { }

Check notice

Code scanning / CodeQL

More than one statement per line Note

This line contains 2 statements; only one is allowed.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@bocchino
Copy link
Collaborator

Function implementations in the header file are fine if they are short functions or they are part of a template.

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I am happy given @bocchino's response. @bocchino are you ready to merge this?

@bocchino bocchino self-requested a review February 14, 2024 03:00
Copy link
Collaborator

@bocchino bocchino left a comment

Choose a reason for hiding this comment

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

Looks good!

@LeStarch LeStarch merged commit f18e540 into nasa:devel Feb 14, 2024
34 checks passed
@LeStarch LeStarch added the Update Instructions Needed Need to add instructions in the release notes for updates. label Feb 14, 2024
@JohanBertrand JohanBertrand deleted the Fix-temporary-a_-prefix branch April 29, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Instructions Needed Need to add instructions in the release notes for updates.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants