This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Change strcpy to std::copy
ClosedPublic

Authored by 1lyasm on Mar 19 2023, 7:52 AM.

Diff Detail

Unit TestsFailed

Event Timeline

1lyasm created this revision.Mar 19 2023, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 7:52 AM
1lyasm requested review of this revision.Mar 19 2023, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2023, 7:52 AM
nikic requested changes to this revision.Mar 19 2023, 8:11 AM
nikic added inline comments.
llvm/lib/Object/COFFImportFile.cpp
89

This isn't really a sensible way to use strncpy -- in fact, I'm pretty sure that this is buggy and should be S.size() + 1, otherwise we're not copying the trailing null byte, which is important in this context.

Explicitly specifying the length does make sense (as we already know it here), but in that case we should just use memcpy, or, to make it more idiomatic for C++, std::copy.

This revision now requires changes to proceed.Mar 19 2023, 8:11 AM
1lyasm updated this revision to Diff 506410.Mar 19 2023, 11:19 AM
1lyasm retitled this revision from Change strcpy to strncpy to Change strcpy to std::copy.

Remove strncpy and add std::copy

1lyasm added a comment.EditedMar 19 2023, 11:21 AM

Generated assembly for strcpy is smaller than that for std::copy, but there is 2 calls in std::copy and 3 calls in strcpy. Could not evaluate performance by just looking at assemblies.

1lyasm marked an inline comment as done.Mar 20 2023, 10:57 AM
nikic accepted this revision.Mar 20 2023, 10:59 AM

LGTM. Assuming you don't have commit access, can you tell me the Name <email> to use for the commit?

This revision is now accepted and ready to land.Mar 20 2023, 10:59 AM

Ilyas Mustafazade <il.mystafa@gmail.com>
Thank you.

1lyasm retitled this revision from Change strcpy to std::copy to [NFC] Change strcpy to std::copy.Mar 20 2023, 2:35 PM
This revision was automatically updated to reflect the committed changes.