Skip to content

Commit f995005

Browse files
salinas-miguelsalinas-miguel
committed
address feedback
1 parent 2fdd5e6 commit f995005

File tree

1 file changed

+47
-56
lines changed

1 file changed

+47
-56
lines changed

‎tools/swift-stdlib-tool/swift-stdlib-tool.cpp

+47-56
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const char *usage =
6565
;
6666

6767
#include <sys/types.h>
68+
#include <sys/param.h>
6869
#include <sys/uio.h>
6970
#include <sys/stat.h>
7071
#include <stdio.h>
@@ -423,8 +424,8 @@ ssize_t pread_all(int fd, void *buf, size_t count, off_t offset)
423424

424425
template <typename T>
425426
int parse_macho(int fd, uint32_t offset, uint32_t size,
426-
void (^dylibVisitor)(std::filesystem::path path),
427-
void (^uuidVisitor)(uuid_t uuid))
427+
void (^dylibVisitor)(std::filesystem::path const &path),
428+
void (^uuidVisitor)(uuid_t const uuid))
428429
{
429430
ssize_t readed;
430431

@@ -473,9 +474,7 @@ void (^dylibVisitor)(std::filesystem::path path),
473474
else if (uuidVisitor && cmd->cmd() == LC_UUID) {
474475
macho_uuid_command<T>* uuid_cmd = (macho_uuid_command<T>*)cmd;
475476
if (uuid_cmd->cmdsize() < sizeof(uuid_command)) continue;
476-
uuid_t uuid;
477-
memcpy(uuid, uuid_cmd->uuid(), 16);
478-
uuidVisitor(uuid);
477+
uuidVisitor(uuid_cmd->uuid());
479478
}
480479
}
481480
}
@@ -486,8 +485,8 @@ void (^dylibVisitor)(std::filesystem::path path),
486485

487486

488487
int parse_macho(int fd, uint32_t offset, uint32_t size,
489-
void (^dylibVisitor)(std::filesystem::path path),
490-
void (^uuidVisitor)(uuid_t uuid))
488+
void (^dylibVisitor)(std::filesystem::path const &path),
489+
void (^uuidVisitor)(uuid_t const uuid))
491490
{
492491
uint32_t magic;
493492
if (size < sizeof(magic)) return log_vv("file is too small");
@@ -514,8 +513,8 @@ int parse_macho(int fd, uint32_t offset, uint32_t size,
514513

515514

516515
int parse_fat(int fd, off_t fsize, char *buffer, size_t size,
517-
void (^dylibVisitor)(std::filesystem::path path),
518-
void (^uuidVisitor)(uuid_t uuid))
516+
void (^dylibVisitor)(std::filesystem::path const &path),
517+
void (^uuidVisitor)(uuid_t const uuid))
519518
{
520519
uint32_t magic;
521520

@@ -608,8 +607,8 @@ int parse_fat(int fd, off_t fsize, char *buffer, size_t size,
608607
}
609608

610609

611-
void process(std::filesystem::path path, void(^dylibVisitor)(std::filesystem::path),
612-
void (^uuidVisitor)(uuid_t))
610+
void process(std::filesystem::path const& path, void(^dylibVisitor)(std::filesystem::path const&),
611+
void (^uuidVisitor)(uuid_t const))
613612
{
614613
log_vv("Scanning %s...", path.c_str());
615614

@@ -644,12 +643,10 @@ bool operator <= (const struct timespec &lhs, const struct timespec &rhs)
644643

645644
// This executable's own path.
646645
std::filesystem::path self_executable = []() -> std::filesystem::path {
647-
uint32_t len = 0;
648-
_NSGetExecutablePath(NULL, &len);
649-
std::vector<char> buffer;
650-
buffer.reserve(len);
651-
_NSGetExecutablePath(buffer.data(), &len);
652-
return buffer.data();
646+
char path[MAXPATHLEN] = {0};
647+
uint32_t len = sizeof(path);
648+
_NSGetExecutablePath(path, &len);
649+
return std::filesystem::path(path);
653650
}();
654651

655652

@@ -695,38 +692,36 @@ int xcrunToolCommand(std::vector<std::string> commandAndArguments, XcrunToolBloc
695692
const char *launchPath = "/usr/bin/xcrun";
696693

697694
// Tell xcrun to search our toolchain first.
698-
const char *arguments[commandAndArguments.size() + 5];
699-
arguments[0] = launchPath;
700-
arguments[1] = "--toolchain";
701-
arguments[2] = self_toolchain.c_str();
702-
int argsCount = 3;
695+
std::vector<const char *> arguments;
696+
arguments.push_back(launchPath);
697+
arguments.push_back("--toolchain");
698+
arguments.push_back(self_toolchain.c_str());
703699

704700
// Tell xcrun to print its command if we are very verbose.
705701
if (Verbose > 1) {
706-
arguments[3] = "--log";
707-
argsCount += 1;
702+
arguments.push_back("--log");
708703
}
709704

710705
for (auto const &string : commandAndArguments) {
711-
arguments[argsCount++] = string.c_str();
706+
arguments.push_back(string.c_str());
712707
}
713-
arguments[argsCount] = NULL;
708+
arguments.push_back(NULL);
714709

715710
int outPipe[2];
716711
int errPipe[2];
717712

718713
pipe(outPipe);
719714
pipe(errPipe);
720715

721-
log_v(" %s", arguments[0]);
716+
log_v(" %s", launchPath);
722717

723718
int childPid = fork();
724719

725720
if (childPid == 0) {
726721
dup2(outPipe[1], STDOUT_FILENO);
727722
dup2(errPipe[1], STDERR_FILENO);
728723

729-
execv(launchPath, (char *const *)arguments);
724+
execv(launchPath, (char *const *)arguments.data());
730725
}
731726

732727
// Read stdout and stderr in parallel, then wait for the task
@@ -791,37 +786,36 @@ copyFile(std::filesystem::path src, std::filesystem::path dst, bool stripBitcode
791786
}
792787
}
793788

794-
std::string uuidString(uuid_t uuid) {
789+
std::string uuidString(uuid_t const uuid) {
795790
char buffer[37];
796791
uuid_unparse(uuid, buffer);
797792
return buffer;
798793
}
799794

800795
void copyLibraries(std::filesystem::path src_dir, std::filesystem::path dst_dir,
801796
std::unordered_map<std::string,
802-
std::unordered_set<std::string>> libs,
797+
std::unordered_set<std::string>> const &libs,
803798
bool stripBitcode)
804799
{
805800
mkpath_np(dst_dir.c_str(), S_IRWXU | S_IRWXG | S_IRWXO);
806801

807-
for (auto const &pair : libs) {
808-
std::filesystem::path src = src_dir/pair.first;
809-
std::filesystem::path dst = dst_dir/pair.first;
802+
for (auto const &[lib, srcUUIDs]: libs) {
803+
std::filesystem::path src = src_dir/lib;
804+
std::filesystem::path dst = dst_dir/lib;
810805

811806
// Compare UUIDs of src and dst and don't copy if they're the same.
812807
// Do not use mod times for this task: the dst copy gets code-signed
813808
// and bitcode-stripped so it can look newer than it really is.
814-
auto const &srcUUIDs = pair.second;
815809
__block std::unordered_set<std::string> dstUUIDs;
816-
process(dst, NULL, ^(uuid_t uuid) {
810+
process(dst, NULL, ^(uuid_t const uuid) {
817811
dstUUIDs.insert(uuidString(uuid));
818812
});
819813

820814
std::string srcUUIDsString;
821815
srcUUIDsString.reserve(37 * srcUUIDs.size());
822816

823817
for (auto const &uuidString : srcUUIDs) {
824-
srcUUIDsString.append(uuidString + " ");
818+
srcUUIDsString.append(uuidString + std::string(" "));
825819
}
826820

827821
std::string dstUUIDsString;
@@ -836,14 +830,14 @@ void copyLibraries(std::filesystem::path src_dir, std::filesystem::path dst_dir,
836830

837831
if (srcUUIDs == dstUUIDs) {
838832
log_v("%s is up to date at %s",
839-
pair.first.c_str(), dst.c_str());
833+
lib.c_str(), dst.c_str());
840834
continue;
841835
}
842836

843837
// Perform the copy.
844838

845839
log_v("Copying %s from %s to %s",
846-
pair.first.c_str(),
840+
lib.c_str(),
847841
src_dir.c_str(),
848842
dst_dir.c_str());
849843

@@ -961,14 +955,13 @@ int main(int argc, const char *argv[])
961955
// Neither src_dir nor platform is set. Die.
962956
fail_usage("At least one of --source-libraries and --platform "
963957
"must be set.");
964-
}
965-
else if (src_dir.empty()) {
958+
} else if (src_dir.empty()) {
966959
// platform is set but src_dir is not.
967960
// Use platform to set src_dir relative to us.
968961
src_dir = self_executable.parent_path().parent_path()/
969962
"lib"/"swift-5.0"/platform;
970963
} else if (platform.empty()) {
971-
// src_dir is set but platform is not.
964+
// src_dir is set but platform is not.
972965
// Pick platform from src_dir's name.
973966
platform = src_dir.filename();
974967
}
@@ -1008,8 +1001,8 @@ int main(int argc, const char *argv[])
10081001
__block std::unordered_map<std::string,
10091002
std::unordered_set<std::string>> swiftLibs;
10101003
for (auto const &path : executables) {
1011-
process(path,
1012-
^(std::filesystem::path linkedLib) {
1004+
process(path,
1005+
^(std::filesystem::path const &linkedLib) {
10131006
auto const linkedSrc = src_dir/linkedLib;
10141007
if (std::filesystem::exists(linkedSrc)) {
10151008
swiftLibs[linkedLib] = std::unordered_set<std::string>();
@@ -1022,15 +1015,15 @@ int main(int argc, const char *argv[])
10221015
// Also collect the Swift libraries' UUIDs.
10231016
__block std::vector<std::string> worklist;
10241017
worklist.reserve(swiftLibs.size());
1025-
for (auto const &pair : swiftLibs) {
1026-
worklist.push_back(pair.first);
1018+
for (auto const &[lib, _] : swiftLibs) {
1019+
worklist.push_back(lib);
10271020
}
10281021
while (worklist.size()) {
10291022
auto const &lib = worklist.back();
10301023
worklist.pop_back();
10311024
auto const path = src_dir/lib;
1032-
process(path,
1033-
^(std::filesystem::path linkedLib) {
1025+
process(path,
1026+
^(std::filesystem::path const &linkedLib) {
10341027
auto const linkedSrc = src_dir/linkedLib;
10351028
if (swiftLibs.count(linkedLib) == 0 &&
10361029
std::filesystem::exists(linkedSrc))
@@ -1039,7 +1032,7 @@ int main(int argc, const char *argv[])
10391032
worklist.push_back(linkedLib);
10401033
}
10411034
},
1042-
^(uuid_t uuid) {
1035+
^(uuid_t const uuid) {
10431036
swiftLibs[lib].insert(uuidString(uuid));
10441037
});
10451038
}
@@ -1057,15 +1050,15 @@ int main(int argc, const char *argv[])
10571050

10581051
// Collect dependencies of --resource-library libs.
10591052
worklist.clear();
1060-
for (auto const &pair : swiftLibsForResources) {
1061-
worklist.push_back(pair.first);
1053+
for (auto const &[lib, _] : swiftLibsForResources) {
1054+
worklist.push_back(lib);
10621055
}
10631056
while (worklist.size()) {
10641057
auto const &lib = worklist.back();
10651058
worklist.pop_back();
10661059
auto const path = src_dir/lib;
10671060
process(path,
1068-
^(std::filesystem::path linkedLib) {
1061+
^(std::filesystem::path const &linkedLib) {
10691062
auto const linkedSrc = src_dir/linkedLib;
10701063
if (swiftLibsForResources.count(linkedLib) == 0 &&
10711064
std::filesystem::exists(linkedSrc))
@@ -1074,7 +1067,7 @@ int main(int argc, const char *argv[])
10741067
worklist.push_back(linkedLib);
10751068
}
10761069
},
1077-
^(uuid_t uuid) {
1070+
^(uuid_t const uuid) {
10781071
swiftLibsForResources[lib].insert(uuidString(uuid));
10791072
});
10801073
}
@@ -1117,7 +1110,7 @@ int main(int argc, const char *argv[])
11171110
__block bool signedOne = false;
11181111
std::mutex signingLock;
11191112

1120-
for (auto const &pair : swiftLibs) {
1113+
for (auto const &[lib, _] : swiftLibs) {
11211114
// Work around authentication UI problems
11221115
// by signing one synchronously and then signing the rest.
11231116
signingLock.lock();
@@ -1128,13 +1121,11 @@ int main(int argc, const char *argv[])
11281121
// We are the first signer. Hold the lock until we finish.
11291122
}
11301123

1131-
auto const &lib = pair.first;
1132-
auto const dst = dst_dir/lib;
1133-
11341124
// Get the code signature, and copy the dylib to the side
11351125
// to preserve it in case it does not change. We can use
11361126
// this to avoid unnecessary copies during delta installs
11371127
// to devices.
1128+
auto const dst = dst_dir/lib;
11381129
auto const oldSignatureData = query_code_signature(dst);
11391130
const char *tmpFilePath = 0;
11401131
if (!oldSignatureData.empty()) {

0 commit comments

Comments
 (0)