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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

So, what about current solution in this patch to report fatal errors through Expected<>/Error in llvm-objcopy? Is it OK to do things that way? My understanding is that currently only fatal errors are reported and then no need to create handlers at the moment.



================
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.
----------------
jhenderson wrote:
> 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).
OK, I will return AddSection back and fix the style issue in a separate commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987