[PATCH] D120687: [llvm-nm][NFC] remove global variable " std::vector<NMSymbol> SymbolList"

Fri Mar 4 07:08:59 PST 2022

DiggerLin added a comment.

In D120687#3351412 <https://reviews.llvm.org/D120687#3351412>, @jhenderson wrote:

> In D120687#3351246 <https://reviews.llvm.org/D120687#3351246>, @DiggerLin wrote:
>
>> In D120687#3350476 <https://reviews.llvm.org/D120687#3350476>, @jhenderson wrote:
>>
>>> I think you may be doing too much in this patch. I think you should start with a patch that literally just changes from a global SymbolList to a local one that's passed down the stack as required. You can then make other changes on top of that to continue the refactoring we discussed.
>>
>>
>>
>>   Any criterion on how to create a NFC patch?  It is difficult for me the control the size of the NFC patch, I already did three refactor patches on the llvm-nm.cpp before I can reach your comment on the patch https://reviews.llvm.org/D112735 , I always feel sad when you tell me that I need to split a patches into two patches , it is time consume  for me to split the patches after I post a patch.  I need to  change the code, compile and test the separate patches.
>
> In general, you should try to keep patches to a minimum size - prefer lots of individual patches over one larger patch to achieve a goal - you'll rarely, if ever find people complaining about you splitting something up too much! This is because it is much harder to review larger changes versus smaller changes, especially if the review is either busy or is not all that familiar with the area. I'm sorry you feel sad about being asked to do these things - it isn't my intention to upset you. However, small, incremental changes are a good developer habit to get into, whether working in open source, on proprietary code, or on personal projects. This is because it is easier to ensure it is correct when done incrementally. There are a number of good books out there that discuss things like this topic. I'm afraid I'm not good at recommending this sort of thing though. It's also worth reading through LLVM's guidelines on creating and reviewing patches (e.g. https://llvm.org/docs/DeveloperPolicy.html and https://llvm.org/docs/CodeReview.html).
>
> In this particular context, you have at least two different items in this patch: the changes to the symbol list population, and the ObjMemBufs stuff. They don't seem to be intertwined with each other such that you couldn't do one without the other. As such, in the ideal world, you should do them separately.
>
>> And I also have my team work on my hand.
>
> Please remember that reviewers also usually have their own work to do. Reviews are done to help ensure a thriving community where people help each other out. I myself often review things that nobody else ever gets around to, but am pushing the amount of time I can spend on reviews as it is. Keeping things simple for reviewers is important. If you don't, people simply won't review your code and you'll never get it landed.

  Thanks a lot of for your time and professional review comment. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120687