refactor: suppress warnings

[Jinchul81]

Warnings have been suppressed according to the following patterns:

• Applied [[maybe_unused]]
• Explicitly initialize the member variables in struct.
• Unaligned type comparison between signed and unsigned. Converted unsigned to singed.
• Hiding virtual functions in the base class. Declare using base::func.

[wgtmac]

Thanks for working on this! I've left a minor comment. Please also fix the linter errors.

[wgtmac]

We don't need to add {} for everything, at least not for STL containers and std::string. Please try to minimize lines of change like this.

[Jinchul81]

It looks like they are necessary. The following warnings occurred when removing the initialization. Do you have any better idea?

../src/iceberg/test/rest_json_internal_test.cc:774:55: warning: missing initializer for member ‘iceberg::rest::CatalogConfig::endpoints’
../src/iceberg/test/rest_json_internal_test.cc:780:76: warning: missing initializer for member ‘iceberg::rest::CatalogConfig::overrides’

[wgtmac]

I'd suggest adding -Wno-missing-field-initializers to suppress such kind of warnings.

[HuaHuaY]

I think we can remove the parameters' names instead of adding [[maybe_unused]] to keep the code clear.

[Jinchul81]

I am not sure your suggestion seems to be aligned with the original design. These functions overrode the pure virtual functions in Catalog.

[HuaHuaY]

I don't get your point. Removing the names in child class don't change the function signature even though it overrides virtual function.

Result<std::unique_ptr<Table>> InMemoryCatalog::CreateTable(
    const TableIdentifier&, const Schema&, const PartitionSpec&,
    const std::string&,
    const std::unordered_map<std::string, std::string>&) {
  return NotImplemented("create table");
}

[Jinchul81]

@HuaHuaY Let's compare the two options. I prefer to use [[maybe_unused]] rather than omitting the parameter name. Adding the attribute can bring better readability and maintenance when enabling parameters.

[HuaHuaY]

According to C++ core guidelines https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rf-unused, [[maybe_unused]] is used when parameters are conditionally unused.
I think it's the responsibility of who implements this feature in the future to add the name back. If we don't need the variable, we should not give a name to it.

[wgtmac]

I've seen several patterns regarding to this:

void func_a([[may_unused]] int a);
void func_b(int /*a*/);
void func_c(int);

I think void func_b(int /*a*/) strikes a good balance.

[HuaHuaY]

In general, we should convert small types to large types. There are some similar codes in this PR that need to be modified.

[Jinchul81]

Let me rewrite the change like this:

      if (target_fixed_type->length() >= 0 &&
          binary_val.size() == static_cast<size_t>(target_fixed_type->length())) {
        return Literal::Fixed(std::move(binary_val));
      }

[HuaHuaY]

I don't think we need to check the length is not smaller than 0.

[Jinchul81]

Okay, let me assume the rhs will never have a negative value.

[HuaHuaY]

@wgtmac Does this method not being used mean we are missing some tests?

[HuaHuaY]

@wgtmac It seems that we should override the Equals methods in Schema instead of adding Schema::Equals(const Schema& other) const at line 108?

[Jinchul81]

@lidavidm Did you intend to introduce non-overriding function Equals in this commit ebacc54?

[Jinchul81]

Let me keep this change because this commit is to refactor the code.

[HuaHuaY]

I didn't make it clear. This and the previous comments are just to remind wgtmac. You don't need to modify these.

[Jinchul81]

I believe we need to treat compiler warnings as errors. May I include it in this PR?

[wgtmac]

I believe we need to treat compiler warnings as errors. May I include it in this PR?

I think it is worth doing if the effort is manageable. In practice, we may need to add some compiler flags to suppress some warnings that are too strict or unnecessary. Sometimes a toolchain upgrade may emerge a lot of new warnings that do not show up in the past.