DRY vs. WET

[aditya-vk]

The [base] robot/manipulator class currently has non-const version of some getter functions as virtual functions and the const version of the function as non-virtual. This is in favor of the DRY sentiment [https://en.wikipedia.org/wiki/Don%27t_repeat_yourself]

This, however, hides the [non-const] base class functions of the same name as the [const] functions in the derived class requiring explicit exposing of the base class function via a using::baseClass::functionName statement in the derived class. It is very easy for the developer to forget these statements requiring iterations of corrections to the code.

Proposal is to DRY if the implementation is complex and WET if trivial. For instance functions like getMetaSkeleton() are trivial and should probably be virtual and implemented in each of the derived classes.

The current status is DRY + explicit un-hiding of base class functions. Note that whoever works on this issue, should also modify Libherb accordingly.

[jslee02]

This suggestion sounds good to me. Could we have this in STYLE.md as well?

[psigen]

I'm not sure I quite follow the implications here. Maybe you could point to some examples of when this is bad?

For instance functions like getMetaSkeleton() are trivial and should probably be virtual and implemented in each of the derived classes.

Even if they are trivial, I don't see why we should be implementing these in each derived class unless they are changing somehow.

[jslee02]

@psigen At first we also intended to apply DRY whenever applicable, but we encountered a rather tricky case. Let me take a simple example:

class A {
public:
  void foo() { foo(0); }
  virtual void foo(int val) = 0;
};

class B : public A {
public:
  // using A::foo; // Expose A::foo to B

  void foo(int val) override;
};

// main.cpp
B b;
b.foo(); // Error w/o adding "using A::foo" to B

In the example, we make A::foo() to call A::foo(int) in spirit of DRY, but this requires using A::foo in B. Otherwise, we couldn't call A::foo() through B. This requirement is fine, it's rather error-prone because the compiler wouldn't explicitly say using A::foo is necessary when it's missing.

@aditya-vk basically suggest making both of A::foo() and A::foo(int) as pure virtual functions in this example so that we can explicitly know the problem when they are missing (i.e., not implemented) in B. This is somewhat against to DRY, but I think being explicit would outweight DRY when the implementation of A::foo() is reasonably simple (to be redundant).

[psigen]

Thanks for the explanation, @jslee02.

b.foo(); // Error w/o adding "using A::foo" to B

Is this a compile-time error or a runtime error?

@aditya-vk basically suggest making both of A::foo() and A::foo(int) as pure virtual functions in this example

Would this mean that every child class would now need to implement A::foo()? This seems like it could get tricky if we have to change A::foo() -- we would need to carefully track all child classes and create a patch that touches all of them.

I was wondering, is it helpful to simultaneously declare virtual and final in this situation?

class A {
public:
  virtual void foo() final { foo(0); }
  virtual void foo(int val) = 0;
};

class B : public A {
public:
  void foo(int val) override;
};

// main.cpp
B b;
b.foo(); // I think this will work because A::foo is virtual?

[jslee02]

It's a compile-time error due to A::foo() is hidden (see this live code), and unfortunately, the code still doesn't work for the same reason.

Would this mean that every child class would now need to implement A::foo()? This seems like it could get tricky if we have to change A::foo() -- we would need to carefully track all child classes and create a patch that touches all of them.

Yes. So I think it comes down to (1) using using A::foo in B vs. (2) syncing the implementation of the two functions by manual.

Hm, I thought (1) would be more trickier because the compiler doesn't directly say we need using A::foo, but we can catch it in compile-time anyway whereas (2) is not.

Now I'm inclining to (1) rather than (2)...

[psigen]

I am thinking the same thing. While it is annoying that the compiler does not give a better error, it does seem that this is a compile-time issue, meaning that it would never make it past automated tests and into the codebase. Thus it might be addressed by a good FAQ entry describing the issue and a note in the STYLE.md.

Separately, any thoughts on whether simultaneous virtual and final is a useful thing to do?

[jslee02]

Thus it might be addressed by a good FAQ entry describing the issue and a note in the STYLE.md.

:+1:

Separately, any thoughts on whether simultaneous virtual and final is a useful thing to do?

AFAIK, the compiler treats it as a non-virtual function when they are both specified. I read this at somewhere but don't remember where is it. :disappointed:

[aditya-vk]

While it is annoying that the compiler does not give a better error, it does seem that this is a compile-time issue, meaning that it would never make it past automated tests and into the codebase.

Oh! That's a good point! It would never go out with an error. Nice. :+1: I agree it'd be better to have it documented in STYLE.md and not repeat code in each of the derived classes.

final helps prevent overriding and not hiding, right? Using both virtual and final is redundant from what I read. I am not sure how it would help circumvent this situation. :thinking: