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

Enable code analysis, sdl checks, binskim and fix related errors #856

Merged
merged 23 commits into from
Apr 15, 2021

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Apr 8, 2021

Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link

github-actions bot commented Apr 8, 2021

Misspellings found, please review:

  • ruleset
To accept these changes, run the following commands from this repository on this branch
pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"sourc "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"ruleset "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

@yao-msft
Copy link
Contributor Author

yao-msft commented Apr 8, 2021

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@yao-msft
Copy link
Contributor Author

yao-msft commented Apr 8, 2021

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@@ -636,8 +636,8 @@ namespace AppInstaller::CLI::Workflow
changes.size(),
findByManifest.Matches.size(),
packagesInBoth.size(),
toLog ? static_cast<std::string_view>(toLog->GetProperty(PackageVersionProperty::Name)) : "",
toLog ? static_cast<std::string_view>(toLog->GetProperty(PackageVersionProperty::Version)) : "",
toLog ? static_cast<std::string>(toLog->GetProperty(PackageVersionProperty::Name)) : "",
Copy link
Member

Choose a reason for hiding this comment

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

Why here but not the two lines below? The difference is rvalue vs lvalue, but the lifetime of the rvalue is until the end of the statement. So converting to string_view is just as valid. This is either a mistake/overcaution in the analysis, or an error in my understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code analysis error is C26449( https://docs.microsoft.com/en-us/cpp/code-quality/c26449?view=msvc-160 )
It specifically checks view created from temporary. It is most likely an overcaution because we immediately use the view within the statement. This is to make code analysis happy.


In reply to: 613644294 [](ancestors = 613644294)

Copy link
Member

Choose a reason for hiding this comment

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

I think the ternary is confusing it, since that link specifically calls out:

Temporaries created for function call arguments are not flagged. It is safe to pass spans from such temporaries if target functions don’t retain data pointers in external variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants