[PATCH] D158285: [NFC][CLANG] Fix wrong orders of function arguments positions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D158285#4603521 <https://reviews.llvm.org/D158285#4603521>, @steakhal wrote:

> In D158285#4603477 <https://reviews.llvm.org/D158285#4603477>, @aaron.ballman wrote:
>
>>> I've seen a few similar patches from you @Manna and probably some related folks.
>>> Could you please clarify the motivation and the expectations? Or feel free to point me to the Discuss post around the subject if I happened to miss it.
>>
>> Intel has been addressing issues found by a static analysis tool and when we find the issue exists in community Clang, we address it upstream. Part of addressing the issue is figuring out whether the issue is actually a false positive or not; we try to limit the changes to only true positives (but some will sometimes slip through the cracks). I don't believe there's been a Discourse post on the topic as it's expected to be regular maintenance work instead of something special.
>
> Thanks for clarifying. It does resonate with me, and it is an important subject to work on. However, I sometimes feel that a little more effort in understanding the context where the suggestion is raised before review would sort out some of the FPs I usually encounter of such patches.
> I find this important because it's difficult to allocate time for doing this.
>
> To me, an important metric of a quality patch is if the title and the summary describe why we have this patch, and what we do about it. Maybe, if it matters, also describe what the author tried and didn't work and why.
> An implicit benefit of this is that it helps the author to reflect and form a deeper understanding of the issue at hand (to be able to describe it).
>
> In addition to this, if it's a behavioral change (like this), it's valuable to demonstrate the bug with a test case covering the changed parts.
> I'm not enforcing this, as it doesn't always make sense. But thinking about it again helps to reflect. At worst, the result of this would materialize in "I don't have a test for this change because XYZ" in the summary of the revision.
> Following these steps would improve review experience, which should also lead to happier patch authors in the form of quick and to-the-point review comments.
>
> I hope I clarified my standpoint. @aaron.ballman

+1, I agree with the points you're raising. My only concern is that some of your points are easier to address for people who are familiar with the code base but harder for folks who are not as well-versed. e.g., I think test coverage is reasonable to ask for when there are functional changes, but we should be pragmatic if it's a real challenge to devise a test case that trips the exact conditions. We may need to help path authors out with some of this as part of the review process, but your point still stands that the patch authors should try to capture more of the bigger picture before putting the patch up. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285