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

Mon Aug 21 06:54:08 PDT 2023

steakhal added a comment.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158285