[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:321-322
     Object &Obj, SectionPred &RemovePred,
     function_ref<bool(const SectionBase &)> shouldReplace,
-    function_ref<SectionBase *(const SectionBase *)> addSection) {
+    function_ref<Expected<SectionBase *>(const SectionBase *)> AddSection) {
   // Build a list of the debug sections we are going to replace.
----------------
avl wrote:
> jhenderson wrote:
> > I would either replace both these names or neither of them, since these are the variable names already. I think we want to avoid an inconsistent style between `shouldReplace` and `AddSection`.
> My own preference is that it is OK to fix small style issues if they were encountered during making some patch. But it is often rejected with the reason - "It should be separate patch". Thus I am trying to change only lines affected by patch. In this concrete case clang-tidy reported style issue - So I fixed it. If it is OK - I would change  shouldReplace also.
I think it's okay to ignore clang-tidy issues where you aren't modifying the specific bit it's complaining about. However, it's probably just better to fix the style issue for both `function_ref` instances in a separate patch, thinking about it.

What I don't think is okay is to have inconsistent style (I don't mind if it is a precursor patch to fix the style, or just ignoring the clang-tidy warning here).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87987/new/

https://reviews.llvm.org/D87987