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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
