majetideepak commented on code in PR #8470:
URL: https://github.com/apache/incubator-gluten/pull/8470#discussion_r1907761978
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -141,7 +141,7 @@ class ListenableArbitrator : public
velox::memory::MemoryArbitrator {
}
Stats stats() const override {
- Stats stats; // no-op
+ Stats stats{}; // no-op
Review Comment:
not required.
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -125,8 +125,8 @@ class ListenableArbitrator : public
velox::memory::MemoryArbitrator {
uint64_t shrinkCapacity(uint64_t targetBytes, bool allowSpill, bool
allowAbort) override {
velox::memory::ScopedMemoryArbitrationContext ctx{};
- facebook::velox::exec::MemoryReclaimer::Stats status;
- velox::memory::MemoryPool* pool;
+ facebook::velox::exec::MemoryReclaimer::Stats status{};
Review Comment:
This change is not required. `facebook::velox::exec::MemoryReclaimer::Stats`
initializes its fields.
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -195,9 +195,12 @@ std::shared_ptr<ColumnarBatch>
WholeStageResultIterator::next() {
if (task_->isFinished()) {
return nullptr;
}
- velox::RowVectorPtr vector;
+ velox::RowVectorPtr vector = nullptr;
while (true) {
auto future = velox::ContinueFuture::makeEmpty();
+ if (task == nullptr) {
Review Comment:
Why add this check?
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -253,7 +257,10 @@ VeloxMemoryManager::VeloxMemoryManager(const std::string&
kind, std::unique_ptr<
namespace {
MemoryUsageStats collectVeloxMemoryUsageStats(const velox::memory::MemoryPool*
pool) {
- MemoryUsageStats stats;
+ if (pool == nullptr) {
Review Comment:
No related to the CVE.
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -433,7 +433,7 @@ VectorPtr SubstraitVeloxExprConverter::literalsToVector(
// Handle EmptyList and List together since the children could be either
case.
case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList:
case ::substrait::Expression_Literal::LiteralTypeCase::kList: {
- ArrayVectorPtr elements;
+ ArrayVectorPtr elements = nullptr;
Review Comment:
not required. `ArrayVectorPtr ` is a `std::shared_ptr` which is initialized
by default.
Same below for `RowVectorPtr` and `MapVectorPtr `
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -215,7 +215,11 @@ class ArbitratorFactoryRegister {
};
VeloxMemoryManager::VeloxMemoryManager(const std::string& kind,
std::unique_ptr<AllocationListener> listener)
- : MemoryManager(kind), listener_(std::move(listener)) {
+ : MemoryManager(kind) {
+ if (listener == nullptr) {
Review Comment:
Why add this check? Is it related to the CVE?
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -1254,12 +1254,12 @@ core::PlanNodePtr
SubstraitToVeloxPlanConverter::toVeloxPlan(const ::substrait::
// Velox requires Filter Pushdown must being enabled.
bool filterPushdownEnabled = true;
- std::shared_ptr<connector::hive::HiveTableHandle> tableHandle;
+ std::shared_ptr<connector::hive::HiveTableHandle> tableHandle = nullptr;
if (!readRel.has_filter()) {
tableHandle = std::make_shared<connector::hive::HiveTableHandle>(
kHiveConnectorId, "hive_table", filterPushdownEnabled,
connector::hive::SubfieldFilters{}, nullptr);
} else {
- connector::hive::SubfieldFilters subfieldFilters;
+ connector::hive::SubfieldFilters subfieldFilters{};
Review Comment:
not required.
##########
cpp/velox/tests/Substrait2VeloxPlanValidatorTest.cc:
##########
@@ -41,7 +41,7 @@ class Substrait2VeloxPlanValidatorTest : public
exec::test::HiveConnectorTestBas
bool validatePlan(std::string file) {
std::string subPlanPath = FilePathGenerator::getDataFilePath(file);
- ::substrait::Plan substraitPlan;
+ ::substrait::Plan substraitPlan{};
Review Comment:
not required.
##########
cpp/velox/tests/Substrait2VeloxPlanValidatorTest.cc:
##########
@@ -60,7 +60,7 @@ class Substrait2VeloxPlanValidatorTest : public
exec::test::HiveConnectorTestBas
TEST_F(Substrait2VeloxPlanValidatorTest, group) {
std::string subPlanPath = FilePathGenerator::getDataFilePath("group.json");
- ::substrait::Plan substraitPlan;
+ ::substrait::Plan substraitPlan{};
Review Comment:
not required.
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -265,20 +272,25 @@ MemoryUsageStats collectVeloxMemoryUsageStats(const
velox::memory::MemoryPool* p
}
MemoryUsageStats collectGlutenAllocatorMemoryUsageStats(const MemoryAllocator*
allocator) {
- MemoryUsageStats stats;
+ MemoryUsageStats stats{};
stats.set_current(allocator->getBytes());
stats.set_peak(allocator->peakBytes());
return stats;
}
int64_t shrinkVeloxMemoryPool(velox::memory::MemoryManager* mm,
velox::memory::MemoryPool* pool, int64_t size) {
- std::string poolName{pool->root()->name() + "/" + pool->name()};
- std::string logPrefix{"Shrink[" + poolName + "]: "};
- VLOG(2) << logPrefix << "Trying to shrink " << size << " bytes of data...";
- VLOG(2) << logPrefix << "Pool has reserved " << pool->usedBytes() << "/" <<
pool->root()->reservedBytes() << "/"
- << pool->root()->capacity() << "/" << pool->root()->maxCapacity() <<
" bytes.";
- VLOG(2) << logPrefix << "Shrinking...";
- auto shrunken = mm->arbitrator()->shrinkCapacity(pool, 0);
+ if (pool != nullptr) {
Review Comment:
not related to the `Use of Uninitialized Variable` CVE
##########
cpp/velox/substrait/SubstraitToVeloxPlan.cc:
##########
@@ -866,7 +866,7 @@ const core::WindowNode::Frame
SubstraitToVeloxPlanConverter::createWindowFrame(
const ::substrait::Expression_WindowFunction_Bound& upper_bound,
const ::substrait::WindowType& type,
const RowTypePtr& inputType) {
- core::WindowNode::Frame frame;
+ core::WindowNode::Frame frame{};
Review Comment:
not required.
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -312,7 +312,7 @@ std::shared_ptr<const core::ConstantTypedExpr>
SubstraitVeloxExprConverter::lite
std::vector<variant> variants;
variants.reserve(literals.size());
VELOX_CHECK_GE(literals.size(), 0, "List should have at least one item.");
- std::optional<TypePtr> literalType;
+ std::optional<TypePtr> literalType = std::nullopt;
Review Comment:
not required. std::optional is initialized by default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
