Code style

[caclark]

The migration to GTK4 resulted in a lot of unused parameter errors.
I used clang-tidy --fix to remove them

clang-tidy does not remove unused parameters, but encloses them in comments - apparently for documentation reasons.

I can leave them as is, or delete them - whatever the consensus is.

[xsdg]

I think if clang-tidy can detect when they're wrong and update them, I do like that style (that can make it easier to realize that an argument that wasn't being used can actually be helpful). That said, I'd worry about those comments getting out of date, though maybe that's less likely since folks would be in the habit of updating the declaration and definition together.

[qarkai]

I'm fan of [[maybe_unused]]. It allows to find all unused parameters and even functions later.
But I don't mind leaving parameter names commented out during the migration period.

[caclark]

I am not enthusiastic about [[maybe_unused]] - it is a lot of typing.
The phrase is perhaps confusing - "maybe unused" vs "may be unused".
Perhaps it is useful in the complex depths of C++, but that is beyond me.

In the case of Geeqie, it would be used when code is revised and a parameter that was once used is no longer used. In that case the commented out parameter makes more sense.

Finding such places can be done via e.g. '(./*[[:alpha:]].[[:alpha:]].**/' - which is a bit of a headache.

But GTK4 migration is an even bigger headache...

[caclark]

@qarkai @xsdg et al

I locally changed .clang-tidy to include
+ - key: readability-identifier-naming.FunctionCase
+ value: lower_case
+ - key: readability-identifier-naming.MacroDefinitionCase
+ value: UPPER_CASE
+ - key: readability-identifier-naming.VariableCase
+ value: lower_case
+ - key: readability-identifier-naming.VariableIgnoredRegexp
+ value: '^__.*$'

This complies with Geeqie's CODING.md:
Names:
of variables & functions: small_letters
of defines: CAPITAL_LETTERS

I have used Codex quite a lot recently and because of the omission of the above, it has created a number of variables with the wrong case - that will be fixed.

But there is also e.g.
namespace
{
constexpr gint PAN_GROUP_MAX = 16;
} // namespace

which is from @qarkai - and probably more places

clang-tidy logs that as an error.

In some of the test files created by @xsdg , variables such as _fd are used. That is also logged as an error because it does not start with a char, and the above regex only ignores double underscore. This can be avoided by ignoring ./tests but perhaps should not be. However src/third-party should be ignored for code checks.

Are there any comments?

I see no alternative to using AI code to complete the GTK3/GTK4 migration, and in some other cases it has reduced hours of work to minutes - it has also created a pain in the neck at times.

Therefore the more extensive code checks are necessary.

[qarkai]

I haven't change case to reduce noise in commits. Feel free to rename to lower case.
See also https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#renum-caps and https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#res-all_caps.

There is no rule about _ prefix. Closest is https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#nl8-use-a-consistent-naming-style. I suggest to use _ as postfix, e.g. rename _fd to fd_ if this doesn't violate clang-tidy rules.

[xsdg]

So, the coding style I'm used to uses _ prefix and postfix to mean different things. variable_ is a non-public member variable of some class. A public member variable would just be named variable. _variable is a local variable with limited scope, and is used to avoid a conflict/clarify readability in cases where variable already exists for some reason (whether as a class member or, for example, a function argument).

Under tests/, I used _fd because the test harness (which is a class) contains fd (which I did because fd_ wasn't common in the codebase)

But as a slightly contrived example of how all three serve different purposes:

struct Point3d {
 private:
  std::string description_;

 public:
  float x = 0;
  float y = 0;
  float z = 0;

  Point3d(std::string_view desc) : description_(desc) {}
  Point3d(std::string_view desc, float x, float y, float z) : description_(desc), x(x), y(y), z(z) {}

  Point3d normalized() {
    const float max = std::max({std::abs(x), std::abs(y), std::abs(z)});
    float _x = x / max;
    float _y = y / max;
    float _z = z / max;

    return Point3d(description_ + "(norm)", x, y, z);
  }

  void print() {
    std::print("%s: (%f, %f, %f)", description_, x, y, z);
  }
};

[xsdg]

Oh, also, I think constexpr constants should be ALL_CAPS as well. That said, part of what it means for something to be a constexpr constant is semantic. If it's constexpr and in a global context, it should definitely be all caps. But if it has a name that only makes sense in the local scope, I feel like that should still be lower-case. For example:

constexpr MAX_ARGUMENT_PAIR_COUNT = 5;

int sum_of_pair_products(const std::vector<int> &args) {
  // Each two arguments forms one pair.
  assert(args.size % 2 == 0);
  long sum = 0;
  constexpr max_arg_idx = MAX_ARGUMENT_PAIR_COUNT * 2 - 1;
  for (int i = 0; i < args.size() && i < max_arg_idx; i += 2) {
    const int small_sum = args[i] * args[i + 1];
    sum += small_sum;

    if (DEBUG) std::print("idx %d, %d of %d: small sum %d, big sum %ld", i, i+1, max_arg_idx, small_sum, sum);
  }
  return sum;
}