Skip to content

Commit abe754a

Browse files
committed
Enforce that android::base::expected, i.e. Result return values are handled
Ignoring failures can hide errors. Existing noncompliant uses are marked with a TODO comment. This is currently reported as a warning at `bazel build` time, and a clang-tidy failure at `bazel test` time. As far as I can tell, clang compiler warnings automatically are converted into `clang-diagnostic-*` failures in clang-tidy, which we convert to errors in our clang-tidy tests. Properties of this approach: - Applies only to our GitHub repository, and not the google-internal downstream. This avoids us blocking submission of other changes Google's internal source tree. - Doesn't break `bazel build`, `bazel run`, or `debuild` at the time of compiler updates or active development of new code. - Validated at PR submission time on GitHub by the `run-cvd-unit-tests` action which runs clang-tidy and blocks submission. - Possible to reproduce failures with `bazel test`. Bug: b/471073085
1 parent d3bb8f7 commit abe754a

File tree

13 files changed

+44
-20
lines changed

13 files changed

+44
-20
lines changed

base/cvd/build_variables.bzl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,5 @@ COPTS = [
22
"-std=c++17",
33
# https://cs.android.com/android/platform/superproject/main/+/main:build/soong/cc/config/global.go;l=428;drc=27f57506c28cc8e4f6a0c0b8ac22c85aa9140688
44
"-ftrivial-auto-var-init=zero",
5+
"-DNODISCARD_EXPECTED",
56
]

base/cvd/cuttlefish/common/libs/utils/files.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,15 +148,18 @@ bool FileHasContent(const std::string& path) {
148148
Result<void> HardLinkDirecoryContentsRecursively(
149149
const std::string& source, const std::string& destination) {
150150
CF_EXPECTF(IsDirectory(source), "Source '{}' is not a directory", source);
151-
EnsureDirectoryExists(destination, 0755);
151+
152+
// TODO: b/471069557 - diagnose need
153+
Result<void> unused = EnsureDirectoryExists(destination, 0755);
152154

153155
const std::function<bool(const std::string&)> linker =
154156
[&source, &destination](const std::string& filepath) mutable {
155157
std::string src_path = filepath;
156158
std::string dst_path =
157159
destination + "/" + filepath.substr(source.size() + 1);
158160
if (IsDirectory(src_path)) {
159-
EnsureDirectoryExists(dst_path);
161+
// TODO: b/471069557 - diagnose need
162+
Result<void> unused = EnsureDirectoryExists(dst_path);
160163
return true;
161164
}
162165
bool overwrite_existing = true;

base/cvd/cuttlefish/host/commands/casimir_control_server/casimir_controller.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,9 @@ CasimirController::SendBroadcast(std::vector<uint8_t> data, std::string type,
233233
CF_EXPECT(Write(poll_command), "Failed to send broadcast frame");
234234

235235
if (timeout != 0) {
236-
ReadRfPacket(std::chrono::microseconds(timeout));
236+
// TODO: b/471069557 - diagnose unused
237+
Result<std::shared_ptr<std::vector<uint8_t>>> unused =
238+
ReadRfPacket(std::chrono::microseconds(timeout));
237239
}
238240

239241
return std::make_tuple(data, type, crc, bits, bitrate, timeout, power);

base/cvd/cuttlefish/host/commands/cvd/cli/commands/create.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,8 @@ Result<void> CvdCreateCommandHandler::Handle(const CommandRequest& request) {
327327

328328
group.SetAllStates(cvd::INSTANCE_STATE_STOPPED);
329329
group.SetStartTime(CvdServerClock::now());
330-
instance_manager_.UpdateInstanceGroup(group);
330+
// TODO: b/471069557 - diagnose unused
331+
Result<void> unused = instance_manager_.UpdateInstanceGroup(group);
331332

332333
GatherVmInstantiationMetrics(group);
333334

base/cvd/cuttlefish/host/commands/cvd/cli/commands/load_configs.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,8 @@ class LoadConfigsCommand : public CvdCommandHandler {
160160
EnsureDirectoryExists(group.HomeDir(), 0775, /* group_name */ "");
161161
if (!mkdir_res.ok()) {
162162
group.SetAllStates(cvd::INSTANCE_STATE_PREPARE_FAILED);
163-
instance_manager_.UpdateInstanceGroup(group);
163+
// TODO: b/471069557 - diagnose unused
164+
Result<void> unused = instance_manager_.UpdateInstanceGroup(group);
164165
}
165166
CF_EXPECT(std::move(mkdir_res));
166167

@@ -169,7 +170,8 @@ class LoadConfigsCommand : public CvdCommandHandler {
169170
auto fetch_res = executor_.ExecuteOne(fetch_cmd, std::cerr);
170171
if (!fetch_res.ok()) {
171172
group.SetAllStates(cvd::INSTANCE_STATE_PREPARE_FAILED);
172-
instance_manager_.UpdateInstanceGroup(group);
173+
// TODO: b/471069557 - diagnose unused
174+
Result<void> unused = instance_manager_.UpdateInstanceGroup(group);
173175
}
174176
CF_EXPECTF(std::move(fetch_res),
175177
"Failed to fetch build artifacts, check '{}' for details",

base/cvd/cuttlefish/host/commands/cvd/cli/commands/status.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,8 @@ Result<void> CvdStatusCommandHandler::Handle(const CommandRequest& request) {
166166
CF_EXPECT(selector::SelectGroup(instance_manager_, request));
167167
status_array = CF_EXPECT(group.FetchStatus(
168168
std::chrono::seconds(flags.wait_for_launcher_seconds)));
169-
instance_manager_.UpdateInstanceGroup(group);
169+
// TODO: b/471069557 - diagnose unused
170+
Result<void> unused = instance_manager_.UpdateInstanceGroup(group);
170171
} else {
171172
std::pair<LocalInstance, LocalInstanceGroup> pair =
172173
flags.instance_name.empty()
@@ -178,7 +179,8 @@ Result<void> CvdStatusCommandHandler::Handle(const CommandRequest& request) {
178179
LocalInstanceGroup group = pair.second;
179180
status_array.append(CF_EXPECT(instance.FetchStatus(
180181
std::chrono::seconds(flags.wait_for_launcher_seconds))));
181-
instance_manager_.UpdateInstanceGroup(group);
182+
// TODO: b/471069557 - diagnose unused
183+
Result<void> unused = instance_manager_.UpdateInstanceGroup(group);
182184
}
183185

184186
if (flags.print) {

base/cvd/cuttlefish/host/commands/cvd/fetch/extract_image_contents.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ Result<std::vector<std::string>> ExtractImageContents(
5252
} else {
5353
CF_EXPECT(MoveDirectoryContents(image_filepath, target_dir));
5454
// Ignore even if removing directory fails - harmless.
55-
RecursivelyRemoveDirectory(image_filepath);
55+
// TODO: b/471069557 - diagnose unused
56+
Result<void> unused = RecursivelyRemoveDirectory(image_filepath);
5657
}
5758
return files;
5859
}

base/cvd/cuttlefish/host/commands/cvd/fetch/fetch_context.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,8 @@ Result<void> FetchArtifact::ExtractOneTo(const std::string& member_name,
145145
fetch_build_context_.trace_.CompletePhase(std::move(phase),
146146
FileSize(extract_path));
147147

148-
fetch_build_context_.DesparseFiles({local_path});
148+
// TODO: b/471069557 - diagnose unused
149+
Result<void> unused = fetch_build_context_.DesparseFiles({local_path});
149150

150151
return {};
151152
}

base/cvd/cuttlefish/host/commands/cvd/instances/instance_database_helper.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,8 @@ CvdInstanceDatabaseTest::~CvdInstanceDatabaseTest() {
7373

7474
void CvdInstanceDatabaseTest::ClearWorkspace() {
7575
if (!workspace_dir_.empty()) {
76-
RecursivelyRemoveDirectory(workspace_dir_);
76+
// TODO: b/471069557 - diagnose unused
77+
Result<void> unused = RecursivelyRemoveDirectory(workspace_dir_);
7778
}
7879
}
7980

base/cvd/cuttlefish/host/commands/cvd/instances/instance_manager.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,8 @@ Result<void> InstanceManager::StopInstanceGroup(
228228
"\".\nThis can happen if instances are already stopped.\n";
229229
}
230230
group.SetAllStates(cvd::INSTANCE_STATE_STOPPED);
231-
instance_db_.UpdateInstanceGroup(group);
231+
// TODO: b/471069557 - diagnose unused
232+
Result<void> unused = instance_db_.UpdateInstanceGroup(group);
232233
return {};
233234
}
234235

@@ -264,7 +265,8 @@ Result<void> InstanceManager::Clear() {
264265
if (Result<void> res = RemoveFile(config_link);!res.ok()) {
265266
LOG(ERROR) << res.error().FormatForEnv();
266267
}
267-
RemoveGroupDirectory(group);
268+
// TODO: b/471069557 - diagnose unused
269+
Result<void> unused = RemoveGroupDirectory(group);
268270
}
269271
std::cerr << "Stopped all known instances\n";
270272
return {};

0 commit comments

Comments
 (0)