Wunused-member-function in test each commit #31180

1. 
   sr-gi commented at 6:49 pm on October 29, 2024: member

   #31045 introduced -werror to test each commit, which makes things like Wunused-member-function an error now.

   This feels pretty aggressive in the cases where some methods may be defined in isolation in commit A, and then used elsewhere in commit B (following A).

2. 
   jonatack commented at 7:50 pm on October 29, 2024: member

   I empathize that it’s a little bit harsh to raise on this, but unsure if it is an issue in practice. As a reviewer, my preference is for the member or method to be used in the commit where it is added. This helps to more easily understand why it was added, and aids in reviewing the change without needing to flip repeatedly between the commits or git rebasing to squash them together.
3. maflcko added the label Tests on Oct 30, 2024
4. 
   maflcko commented at 7:44 am on October 30, 2024: member

   It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?

   It should also be trivial to remove the error completely from the CI task by specifying Wno-error=unused-member-function?

5. 
   sr-gi commented at 5:33 pm on October 30, 2024: member

   It may be trivial to suppress either way by using https://en.cppreference.com/w/cpp/language/attributes/maybe_unused in the commit?

   It should also be trivial to remove the error completely from the CI task by specifying Wno-error=unused-member-function?

   Agreed on both. The purpose of the issue was mostly to agree on what the expectations are for methods that may be used within the same PR but not within the commit they were defined in.

   Will open a PR adding Wno-error=unused-member-function to test each commit and see if there is consensus on this

6. fanquake closed this on Nov 1, 2024

   ---

Contributors
sr-gi jonatack maflcko

Labels
Tests