Skip to content

Commit

Permalink
Make setContainsErrors() and setIOException() void
Browse files Browse the repository at this point in the history
The self-return isn't used anywhere. Anyway, that idiom is more appropriate for true builders where the information is known up-front; here it works more as a stateful mutation.

Using void simplifies pulling these methods up to a base class in a follow-up CL.

Work toward #19922.

PiperOrigin-RevId: 676531919
Change-Id: Id13f600a167ac13183129a7e87bff97e519adf97
  • Loading branch information
brandjon authored and iancha1992 committed Sep 20, 2024
1 parent f684292 commit f6318a6
Showing 1 changed file with 6 additions and 10 deletions.
16 changes: 6 additions & 10 deletions src/main/java/com/google/devtools/build/lib/packages/Package.java
Original file line number Diff line number Diff line change
Expand Up @@ -1097,8 +1097,6 @@ default boolean precomputeTransitiveLoads() {
@Nullable private String ioExceptionMessage = null;
@Nullable private IOException ioException = null;
@Nullable private DetailedExitCode ioExceptionDetailedExitCode = null;
// TODO(#19922): Consider having separate containsErrors fields on Metadata and Package. In that
// case, this field is replaced by the one on Metadata.
private boolean containsErrors = false;
// A package's FailureDetail field derives from the events on its Builder's event handler.
// During package deserialization, those events are unavailable, because those events aren't
Expand Down Expand Up @@ -1673,11 +1671,11 @@ void setComputationSteps(long n) {
pkg.computationSteps = n;
}

Builder setIOException(IOException e, String message, DetailedExitCode detailedExitCode) {
void setIOException(IOException e, String message, DetailedExitCode detailedExitCode) {
this.ioException = e;
this.ioExceptionMessage = message;
this.ioExceptionDetailedExitCode = detailedExitCode;
return setContainsErrors();
setContainsErrors();
}

/**
Expand All @@ -1688,13 +1686,11 @@ Builder setIOException(IOException e, String message, DetailedExitCode detailedE
// TODO(bazel-team): For simplicity it would be nice to replace this with
// getLocalEventHandler().hasErrors(), since that would prevent the kind of inconsistency where
// we have reported an ERROR event but not called setContainsErrors(), or vice versa.
@CanIgnoreReturnValue
public Builder setContainsErrors() {
public void setContainsErrors() {
// TODO(bazel-team): Maybe do Preconditions.checkState(localEventHandler.hasErrors()).
// Maybe even assert that it has a FailureDetail, though that's a linear scan unless we
// customize the event handler.
containsErrors = true;
return this;
this.containsErrors = true;
}

public boolean containsErrors() {
Expand All @@ -1720,7 +1716,7 @@ FailureDetail getFailureDetail() {
if (detailedExitCode != null && detailedExitCode.getFailureDetail() != null) {
return detailedExitCode.getFailureDetail();
}
if (containsErrors) {
if (containsErrors()) {
if (undetailedEvents == null) {
undetailedEvents = new ArrayList<>();
}
Expand Down Expand Up @@ -2106,7 +2102,7 @@ private void addRuleInternal(Rule rule) {
}
addOrReplaceTarget(rule);
if (rule.containsErrors()) {
this.setContainsErrors();
setContainsErrors();
}
}

Expand Down

0 comments on commit f6318a6

Please sign in to comment.