[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 30 00:23:33 PDT 2020
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
More information about the llvm-commits
mailing list