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.

Sign up for GitHub

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

Jump to bottom

GH-41994 [C++]: kernel.cc: Remove defaults on switch so that compiler can check full enum coverage for us #41995

Merged
merged 2 commits into from
Jun 6, 2024
Merged

GH-41994 [C++]: kernel.cc: Remove defaults on switch so that compiler can check full enum coverage for us #41995

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84b44d8
kernel.cc: Remove defaults on switch so that compiler can check full …
felipecrv Jun 5, 2024
997b35f
kernel.cc: Make the ANY_TYPE handling less confusing
felipecrv Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 24 additions & 20 deletions cpp/src/arrow/compute/kernel.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ size_t InputType::Hash() const {
case InputType::EXACT_TYPE:
hash_combine(result, type_->Hash());
break;
default:
case InputType::ANY_TYPE:
case InputType::USE_TYPE_MATCHER:
break;
}
return result;
Expand All @@ -378,10 +379,8 @@ std::string InputType::ToString() const {
break;
case InputType::USE_TYPE_MATCHER: {
ss << type_matcher_->ToString();
} break;
default:
DCHECK(false);
break;
}
}
return ss.str();
}
Expand All @@ -400,9 +399,8 @@ bool InputType::Equals(const InputType& other) const {
return type_->Equals(*other.type_);
case InputType::USE_TYPE_MATCHER:
return type_matcher_->Equals(*other.type_matcher_);
default:
return false;
}
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment about "must not be reached" or something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be reached if kind_ is bogus memory. I could remove this line altogether and let poison (true/false randomly) be returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming @wesm was intentional in having the default case here to protect against that kind of UB.

I checked this on godbolt: https://godbolt.org/z/qEaGMGdzh

}

bool InputType::Matches(const DataType& type) const {
Expand All @@ -411,21 +409,23 @@ bool InputType::Matches(const DataType& type) const {
return type_->Equals(type);
case InputType::USE_TYPE_MATCHER:
return type_matcher_->Matches(type);
default:
// ANY_TYPE
return true;
case InputType::ANY_TYPE:
break;
Copy link
Member

Choose a reason for hiding this comment

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

It's not a strong opinion but I like using return true here for consistency of other cases in this switch:

Suggested change
break;
return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'm changing this to true and false after the switch.

}
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that if kind_ is poisoned memory, we return true instead of poison (true or false non-deterministically). Making the code identical to the previous version but without the default case.

}

bool InputType::Matches(const Datum& value) const {
switch (value.kind()) {
case Datum::NONE:
case Datum::RECORD_BATCH:
case Datum::TABLE:
DCHECK(false) << "Matches expects ARRAY, CHUNKED_ARRAY or SCALAR";
return false;
case Datum::ARRAY:
case Datum::CHUNKED_ARRAY:
case Datum::SCALAR:
break;
default:
DCHECK(false);
return false;
}
return Matches(*value.type());
}
Expand All @@ -445,11 +445,13 @@ const TypeMatcher& InputType::type_matcher() const {

Result<TypeHolder> OutputType::Resolve(KernelContext* ctx,
const std::vector<TypeHolder>& types) const {
if (kind_ == OutputType::FIXED) {
return type_.get();
} else {
return resolver_(ctx, types);
switch (kind_) {
case OutputType::FIXED:
return type_;
case OutputType::COMPUTED:
break;
}
return resolver_(ctx, types);
Comment on lines +452 to +454
Copy link
Member

Choose a reason for hiding this comment

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

How about returning an error by default?

Suggested change
break;
}
return resolver_(ctx, types);
return resolver_(ctx, types);
}
return Status::NotImplemented("Must not happen"); // UnknownError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overkill. That return Status would only ever be reached if kind_ is poisoned memory: in that case all that we need is consistent behavior to not go crazy debugging.

}

const std::shared_ptr<DataType>& OutputType::type() const {
Expand All @@ -463,11 +465,13 @@ const OutputType::Resolver& OutputType::resolver() const {
}

std::string OutputType::ToString() const {
if (kind_ == OutputType::FIXED) {
return type_->ToString();
} else {
return "computed";
switch (kind_) {
case OutputType::FIXED:
return type_->ToString();
case OutputType::COMPUTED:
break;
}
return "computed";
Comment on lines +471 to +474
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case OutputType::COMPUTED:
break;
}
return "computed";
case OutputType::COMPUTED:
return "computed";
}
return "unknown";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same point as above.

}

// ----------------------------------------------------------------------
Expand Down
Loading