Addressing lints and comments. (facebookincubator#6579)

Summary: Pull Request resolved: facebookincubator#6579 Addressing lints and comments from D49117889 (facebookincubator#6531). - Using TypePtr instead of std::shared_ptr<const Type> in some places. - Throwing for bad indices in ArrayType::nameOf() and MapType::nameOf(). - Moving TypePtr in some places instead of copying. - Fixing 'use after move'. Reviewed By: Yuhta Differential Revision: D49291987 fbshipit-source-id: fbc92b32c0db9b3b284fa9c683fa78d7feeb43c3
mbasmanova · Sep 15, 2023 · 1f9400e · 1f9400e
1 parent 9212798
commit 1f9400e
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 42 deletions.
diff --git a/velox/dwio/dwrf/reader/ReaderBase.h b/velox/dwio/dwrf/reader/ReaderBase.h
@@ -123,7 +123,7 @@ class ReaderBase {
     return schema_;
   }
 
-  void setSchema(RowTypePtr& newSchema) {
+  void setSchema(const RowTypePtr& newSchema) {
     schema_ = newSchema;
   }
 

diff --git a/velox/type/Type.cpp b/velox/type/Type.cpp
@@ -223,7 +223,7 @@ std::string ArrayType::toString() const {
 }
 
 const TypePtr& ArrayType::childAt(uint32_t idx) const {
-  VELOX_USER_CHECK_EQ(idx, 0, "List type should have only one child");
+  VELOX_USER_CHECK_EQ(idx, 0, "Array type should have only one child");
   return elementType();
 }
 
@@ -264,6 +264,17 @@ const TypePtr& MapType::childAt(uint32_t idx) const {
       idx);
 }
 
+const char* MapType::nameOf(uint32_t idx) const {
+  if (idx == 0) {
+    return "key";
+  } else if (idx == 1) {
+    return "value";
+  }
+  VELOX_USER_FAIL(
+      "Map type should have only two children. Tried to get name of child '{}'",
+      idx);
+}
+
 MapType::MapType(TypePtr keyType, TypePtr valueType)
     : keyType_{std::move(keyType)},
       valueType_{std::move(valueType)},

diff --git a/velox/type/Type.h b/velox/type/Type.h
@@ -435,8 +435,7 @@ struct TypeParameter {
 ///   IntegerType    ArrayType
 ///                     |
 ///                   BigintType
-class Type : public Tree<const std::shared_ptr<const Type>>,
-             public velox::ISerializable {
+class Type : public Tree<const TypePtr>, public velox::ISerializable {
  public:
   explicit Type(TypeKind kind) : kind_{kind} {}
 
@@ -776,22 +775,22 @@ class UnknownType : public TypeBase<TypeKind::UNKNOWN> {
 
 class ArrayType : public TypeBase<TypeKind::ARRAY> {
  public:
-  explicit ArrayType(std::shared_ptr<const Type> child);
+  explicit ArrayType(TypePtr child);
 
   explicit ArrayType(
       std::vector<std::string>&& /*names*/,
-      std::vector<std::shared_ptr<const Type>>&& types)
-      : ArrayType(types[0]) {}
+      std::vector<TypePtr>&& types)
+      : ArrayType(std::move(types[0])) {}
 
-  const std::shared_ptr<const Type>& elementType() const {
+  const TypePtr& elementType() const {
     return child_;
   }
 
   uint32_t size() const override {
     return 1;
   }
 
-  std::vector<std::shared_ptr<const Type>> children() const {
+  std::vector<TypePtr> children() const {
     return {child_};
   }
 
@@ -802,7 +801,8 @@ class ArrayType : public TypeBase<TypeKind::ARRAY> {
   const std::shared_ptr<const Type>& childAt(uint32_t idx) const override;
 
   const char* nameOf(uint32_t idx) const {
-    return idx == 0 ? "element" : "<invalid>";
+    VELOX_USER_CHECK_EQ(idx, 0, "Array type should have only one child");
+    return "element";
   }
 
   std::string toString() const override;
@@ -816,34 +816,32 @@ class ArrayType : public TypeBase<TypeKind::ARRAY> {
   }
 
  protected:
-  std::shared_ptr<const Type> child_;
+  TypePtr child_;
   const std::vector<TypeParameter> parameters_;
 };
 
 class MapType : public TypeBase<TypeKind::MAP> {
  public:
-  MapType(
-      std::shared_ptr<const Type> keyType,
-      std::shared_ptr<const Type> valueType);
+  MapType(TypePtr keyType, TypePtr valueType);
 
   explicit MapType(
       std::vector<std::string>&& /*names*/,
-      std::vector<std::shared_ptr<const Type>>&& types)
-      : MapType(types[0], types[1]) {}
+      std::vector<TypePtr>&& types)
+      : MapType(std::move(types[0]), std::move(types[1])) {}
 
-  const std::shared_ptr<const Type>& keyType() const {
+  const TypePtr& keyType() const {
     return keyType_;
   }
 
-  const std::shared_ptr<const Type>& valueType() const {
+  const TypePtr& valueType() const {
     return valueType_;
   }
 
   uint32_t size() const override {
     return 2;
   }
 
-  std::vector<std::shared_ptr<const Type>> children() const {
+  std::vector<TypePtr> children() const {
     return {keyType_, valueType_};
   }
 
@@ -853,11 +851,9 @@ class MapType : public TypeBase<TypeKind::MAP> {
 
   std::string toString() const override;
 
-  const std::shared_ptr<const Type>& childAt(uint32_t idx) const override;
+  const TypePtr& childAt(uint32_t idx) const override;
 
-  const char* nameOf(uint32_t idx) const {
-    return idx == 0 ? "key" : idx == 1 ? "value" : "<invalid>";
-  }
+  const char* nameOf(uint32_t idx) const;
 
   bool equivalent(const Type& other) const override;
 
@@ -868,8 +864,8 @@ class MapType : public TypeBase<TypeKind::MAP> {
   }
 
  private:
-  std::shared_ptr<const Type> keyType_;
-  std::shared_ptr<const Type> valueType_;
+  TypePtr keyType_;
+  TypePtr valueType_;
   const std::vector<TypeParameter> parameters_;
 };
 
@@ -1281,50 +1277,45 @@ struct TypeFactory<TypeKind::UNKNOWN> {
 
 template <>
 struct TypeFactory<TypeKind::ARRAY> {
-  static std::shared_ptr<const ArrayType> create(
-      std::shared_ptr<const Type> elementType) {
+  static std::shared_ptr<const ArrayType> create(TypePtr elementType) {
     return std::make_shared<ArrayType>(std::move(elementType));
   }
 };
 
 template <>
 struct TypeFactory<TypeKind::MAP> {
   static std::shared_ptr<const MapType> create(
-      std::shared_ptr<const Type> keyType,
-      std::shared_ptr<const Type> valType) {
-    return std::make_shared<MapType>(keyType, valType);
+      TypePtr keyType,
+      TypePtr valType) {
+    return std::make_shared<MapType>(std::move(keyType), std::move(valType));
   }
 };
 
 template <>
 struct TypeFactory<TypeKind::ROW> {
   static std::shared_ptr<const RowType> create(
       std::vector<std::string>&& names,
-      std::vector<std::shared_ptr<const Type>>&& types) {
+      std::vector<TypePtr>&& types) {
     return std::make_shared<const RowType>(std::move(names), std::move(types));
   }
 };
 
-std::shared_ptr<const ArrayType> ARRAY(std::shared_ptr<const Type> elementType);
+std::shared_ptr<const ArrayType> ARRAY(TypePtr elementType);
 
 std::shared_ptr<const RowType> ROW(
     std::vector<std::string>&& names,
-    std::vector<std::shared_ptr<const Type>>&& types);
+    std::vector<TypePtr>&& types);
 
 std::shared_ptr<const RowType> ROW(
-    std::initializer_list<
-        std::pair<const std::string, std::shared_ptr<const Type>>>&& pairs);
+    std::initializer_list<std::pair<const std::string, TypePtr>>&& pairs);
 
-std::shared_ptr<const RowType> ROW(
-    std::vector<std::shared_ptr<const Type>>&& pairs);
+std::shared_ptr<const RowType> ROW(std::vector<TypePtr>&& pairs);
 
-std::shared_ptr<const MapType> MAP(
-    std::shared_ptr<const Type> keyType,
-    std::shared_ptr<const Type> valType);
+std::shared_ptr<const MapType> MAP(TypePtr keyType, TypePtr valType);
 
 std::shared_ptr<const FunctionType> FUNCTION(
-    std::vector<std::shared_ptr<const Type>>&& argumentTypes,
-    std::shared_ptr<const Type> returnType);
+    std::vector<TypePtr>&& argumentTypes,
+    TypePtr returnType);
 
 template <typename Class>
 std::shared_ptr<const OpaqueType> OPAQUE() {