Address reviews

Author: pmenon

src/codegen/codegen.cpp

@@ -69,17 +69,6 @@ llvm::Value *CodeGen::ConstString(const std::string &str_val,
   return GetBuilder().CreateInBoundsGEP(global_var, {Const32(0), Const32(0)});
 }
 
-llvm::Value *CodeGen::ConstType(const type::Type &type) {
-  auto iter = type_variables_.find(type);
-  if (iter != type_variables_.end()) {
-    return iter->second;
-  }
-  const type::Type t = type;
-  llvm::Value *ret = ConstGenericBytes(&type, sizeof(type), "type");
-  type_variables_.insert(std::make_pair(t, ret));
-  return ret;
-}
-
 llvm::Value *CodeGen::ConstGenericBytes(const void *data, uint32_t length,
                                         const std::string &name) const {
   // Create the constant data array that wraps the input data
@@ -164,6 +153,14 @@ llvm::Value *CodeGen::Printf(const std::string &format,
   auto *printf_fn = LookupBuiltin("printf");
   if (printf_fn == nullptr) {
 #if GCC_AT_LEAST_6
+// In newer GCC versions (i.e., GCC 6+), function attributes are part of the
+// type system and are attached to the function signature. For example, printf()
+// comes with the "noexcept" attribute. Moreover, GCC 6+ will complain when
+// attributes attached to a function (e.g., noexcept()) are not used at
+// their call-site. Below, we use decltype(printf) to get the C/C++ function
+// type of printf(...), but we discard the attributes since we don't need
+// them. Hence, on GCC 6+, compilation will fail without adding the
+// "-Wignored-attributes" flag. So, we add it here only.
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wignored-attributes"
 #endif
@@ -189,6 +186,14 @@ llvm::Value *CodeGen::Memcmp(llvm::Value *ptr1, llvm::Value *ptr2,
   auto *memcmp_fn = LookupBuiltin(kMemcmpFnName);
   if (memcmp_fn == nullptr) {
 #if GCC_AT_LEAST_6
+// In newer GCC versions (i.e., GCC 6+), function attributes are part of the
+// type system and are attached to the function signature. For example, memcmp()
+// comes with the "throw()" attribute, among many others. Moreover, GCC 6+ will
+// complain when attributes attached to a function are not used at their
+// call-site. Below, we use decltype(memcmp) to get the C/C++ function type
+// of memcmp(...), but we discard the attributes since we don't need them.
+// Hence, on GCC 6+, compilation will fail without adding the
+// "-Wignored-attributes" flag. So, we add it here only.
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wignored-attributes"
 #endif

src/include/codegen/codegen.h

@@ -59,9 +59,10 @@ class CppProxyMember {
   uint32_t slot_;
 };
 
-//===----------------------------------------------------------------------===//
-// The main wrapper around LLVM's IR Builder to generate IR
-//===----------------------------------------------------------------------===//
+/**
+ * The main API used to generate code in Peloton. Provides a thin wrapper around
+ * LLVM's IR Builder to generate IR.
+ */
 class CodeGen {
  public:
   /// Constructor and destructor
@@ -89,7 +90,8 @@ class CodeGen {
   }
   llvm::Type *ArrayType(llvm::Type *type, uint32_t num_elements) const;
 
-  /// Constant wrappers for bool, int8, int16, int32, int64, strings, and null
+  /// Functions to return LLVM values for constant boolean, int8, int16, int32,
+  // int64, strings, and null values.
   llvm::Constant *ConstBool(bool val) const;
   llvm::Constant *Const8(int8_t val) const;
   llvm::Constant *Const16(int16_t val) const;
@@ -98,7 +100,6 @@ class CodeGen {
   llvm::Constant *ConstDouble(double val) const;
   llvm::Value *ConstString(const std::string &str_val,
                            const std::string &name) const;
-  llvm::Value *ConstType(const type::Type &type);
   llvm::Value *ConstGenericBytes(const void *data, uint32_t length,
                                  const std::string &name) const;
   llvm::Constant *Null(llvm::Type *type) const;
@@ -195,9 +196,6 @@ class CodeGen {
  private:
   // The context/module where all the code this class produces goes
   CodeContext &code_context_;
-
-  std::unordered_map<type::Type, llvm::Value *, type::TypeHasher,
-                     type::TypeEquality> type_variables_;
 };
 
 }  // namespace codegen

test/function/numeric_functions_test.cpp

@@ -28,9 +28,9 @@ using ::testing::Return;
 namespace peloton {
 namespace test {
 
-class NumericFunctionsTest : public PelotonTest {};
+class NumericFunctionsTests : public PelotonTest {};
 
-TEST_F(NumericFunctionsTest, SqrtTest) {
+TEST_F(NumericFunctionsTests, SqrtTest) {
   const double column_val = 9.0;
   const double expected = sqrt(9.0);
   std::vector<type::Value> args = {
@@ -46,7 +46,7 @@ TEST_F(NumericFunctionsTest, SqrtTest) {
   EXPECT_TRUE(result.IsNull());
 }
 
-TEST_F(NumericFunctionsTest, FloorTest) {
+TEST_F(NumericFunctionsTests, FloorTest) {
   // Testing Floor with DecimalTypes
   std::vector<double> inputs = {9.5, 3.3, -4.4, 0.0};
   std::vector<type::Value> args;
@@ -88,7 +88,7 @@ TEST_F(NumericFunctionsTest, FloorTest) {
   EXPECT_TRUE(result.IsNull());
 }
 
-TEST_F(NumericFunctionsTest, RoundTest) {
+TEST_F(NumericFunctionsTests, RoundTest) {
   std::vector<double> column_vals = {9.5, 3.3, -4.4, -5.5, 0.0};
   std::vector<type::Value> args;
   for (double val : column_vals) {
@@ -104,7 +104,7 @@ TEST_F(NumericFunctionsTest, RoundTest) {
   EXPECT_TRUE(result.IsNull());
 }
 
-TEST_F(NumericFunctionsTest,AbsTestDouble) {
+TEST_F(NumericFunctionsTests,AbsTestDouble) {
   std::vector<double> doubleTestInputs = {9.5, -2.5, -4.4, 0.0};
   std::vector<type::Value> args;
   for (double in : doubleTestInputs) {
@@ -120,7 +120,7 @@ TEST_F(NumericFunctionsTest,AbsTestDouble) {
   EXPECT_TRUE(result.IsNull());
 }
 
-TEST_F(NumericFunctionsTest, AbsTestInt) {
+TEST_F(NumericFunctionsTests, AbsTestInt) {
   std::vector<int64_t> bigIntTestInputs = {-20, -15, -10, 0, 10, 20};
   std::vector<int32_t> intTestInputs = {-20, -15, -10, 0, 10, 20};
   std::vector<int16_t> smallIntTestInputs = {-20, -15, -10, 0, 10, 20};
@@ -157,7 +157,7 @@ TEST_F(NumericFunctionsTest, AbsTestInt) {
   }
 }
 
-TEST_F(NumericFunctionsTest, CeilTestDouble) {
+TEST_F(NumericFunctionsTests, CeilTestDouble) {
   std::vector<double> doubleTestInputs = {-36.0, -35.222, -0.7, -0.5, -0.2,
                                           0.0, 0.2, 0.5, 0.7, 35.2, 36.0,
                                           37.2222};
@@ -174,7 +174,7 @@ TEST_F(NumericFunctionsTest, CeilTestDouble) {
   EXPECT_TRUE(result.IsNull());
 }
 
-TEST_F(NumericFunctionsTest, CeilTestInt) {
+TEST_F(NumericFunctionsTests, CeilTestInt) {
   std::vector<int64_t> bigIntTestInputs = {-20, -15, -10, 0, 10, 20};
   std::vector<int32_t> intTestInputs = {-20, -15, -10, 0, 10, 20};
   std::vector<int16_t> smallIntTestInputs = {-20, -15, -10, 0, 10, 20};

test/include/codegen/testing_codegen_util.h

@@ -52,9 +52,11 @@ struct TempFileHandle {
 };
 
 /**
- * Common base class for all codegen tests. This class four test tables that all
- * the codegen components use. Their ID's are available through the oid_t
- * enumeration.
+ * Common base class for all codegen tests. This class has four test tables
+ * whose IDs and names are stored in test_table_oids and test_table_names,
+ * respectively. The test tables all have the exact schema: column "a" and "b"
+ * are integers, column "c" is a decimal, and column "d" is a varchar. The table
+ * with the highest OID also has a primary key on column "a".
  */
 class PelotonCodeGenTest : public PelotonTest {
  public: