felipecrv commented on PR #35340: URL: https://github.com/apache/arrow/pull/35340#issuecomment-1542266683
@pitrou alright. I simplified the description of the PR. I messed up in my rationale and mixed two good practices that I have internalized and always followed blindly: (1) base class should have `virtual` dtor and derived class should have `~Derived() override` to guard against that base class changing and inadvertently becoming non-virtual [1] (not the case here as the base type class will never loose its virtual members) [1] https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-override.html Sorry to get all language-lawyerly. The real motivation behind the PR is that I saw a class that I had added not following the established practices in the source file. I try to match the existing Arrow style even I don't understand the rationale. There was the LTO issue that I didn't know about after all. Thank you @mapleFU for bringing it up. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
