rchamala wrote: > Thanks @rchamala. I wanted to call out two things: > > * Even though this is a trivial change, there was no urgency to fix this and > merge this without review. If anything it's nicer to let this sit so the > person who provided post-commit review is more likely to see it. I think the > original PR was merged too quickly too, since I provided feedback and it was > merged before I had a chance to look at it. Even though the LLVM policy says > you can merge with one approval, it's more than common courtesy to give > reviewers a chance to see their comments addressed. If it was a one-off I > wouldn't have said anything but I want to make sure this doesn't become a > pattern. > * For trivial commits, please avoid using markdown headers. Rendered on > Github they're not too bad, but in the textual commit message they add > unnecessary noise. (See some of the > [discussion](https://discourse.llvm.org/t/concerns-about-low-quality-prs-being-merged-into-main/89748/) > on Discourse).
Sorry about that @JDevlieghere . I thought I had addressed most of your comments in the original PR. Maybe I misunderstood it when I saw you mentioned that you only had nit comments and scripted plugin is reviewed by other reviewer. Will keep in mind for future PR With regards to the new PR’s , they were failing in CI only on windows, which I cannot test easily locally. So, was in urgency mode and wanted to quick push PR’s to unblock other devs https://github.com/llvm/llvm-project/pull/181498 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
