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

BaseTools: Blackbox header cleanup #10601

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

deeglaze
Copy link
Contributor

@deeglaze deeglaze commented Jan 9, 2025

Description

Some code quality cleanups.

  • Breaking change?
  • Impacts security?
  • Includes tests?

How This Was Tested

Builds

Minor cleanup

Signed-off-by: Dionna Glaze <[email protected]>
Minor cleanup

using namespace std in a header is infectious and therefore not best
practice. The declaration build preprocessor may be disabled, which
causes these error commands to cause build errors.

Signed-off-by: Dionna Glaze <[email protected]>
@@ -77,7 +77,7 @@ class DllExportPCCTS ParserBlackBox {
{
if (f == NULL)
{
cerr << "invalid file pointer\n";
std::cerr << "invalid file pointer\n";
Copy link
Member

Choose a reason for hiding this comment

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

Should the PCCTS_NAMESPACE_STD on line 38 be dropped as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pccts is from third party project. The general rule is to only do the necessary changes for it. Seemly, these changes are for code clean up, not for functionality. So, I don't suggest to merge them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pccts is a code graveyard that I would recommend replacing with a first party implementation that uses modern language tools if you're not willing to take clean-up changes.
Trying to integrate software analysis with the codebase leads to alarms on code like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@deeglaze Since the project to migrate all BaseTools C tools to Python has been abandoned, I want to take a look at modernizing all the code including replacing pccts.

This header is not included in any c program, and its use should not be
dependent on whether the build is using namespace std.

Signed-off-by: Dionna Glaze <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants