jdoerfert added subscribers: raghavendhra, jhuber6, anchu-rajendran, kiranktp, DavidTruby, ronlieb, kiranchandramohan. jdoerfert added a comment.
In D79675#2058657 <https://reviews.llvm.org/D79675#2058657>, @fghanim wrote: > I am moving on because we are not getting anywhere. However, There are few > things I need to point out very quickly. > > > I fail to see the point in committing for example your target type solution > > if we found a more generic version in the meantime. > > We can for sure commit them and then replace them subsequently, but is > > that really helping anyone? It would not be a question if > > they were in, since they are not it seems to me there is no benefit in > > blocking the other patch on them. I mean, the time you worked > > on that part is not "less wasted" if we commit it. TBH, I don't thin it is > > wasted at all but that is a different conversation. > > At one point, you said I was delaying D80222 > <https://reviews.llvm.org/D80222> moments after it was uploaded. Now, D79675 > <https://reviews.llvm.org/D79675> and D79676 > <https://reviews.llvm.org/D79676> , cannot be committed because of the > artificial dependency on that patch. That is not the reason these patches cannot be committed. The reason is that *parts of them* needed changes and another round of review. >> I'm sorry you **feel** I waste your time. I really would not do so on >> purpose. > > It is not a feeling. It is a matter of record, and never said you did so on > purpose. Freudian slip? :p Wouldn't that logic imply that any code replacement causes the original work to be "wasted time"? I assume that is not the case, hence I do not assume you wasted yours (wrt. the code). >> While more reviewers would obviously help, it is known that smaller patches >> do too. > > D79739 <https://reviews.llvm.org/D79739> has been merged with D80222 > <https://reviews.llvm.org/D80222>. I kinda feel bad for the reviewer ;) > You are the code owner of the `OMPBuilder`, who do you suggest as reviewers > that I can add, in the future? As of now, I don't really see anyone else doing reviews. I was hoping you would start reviewing patches. Same for @jhuber6 at some point. Other likely candidates are: @kiranchandramohan, @DavidTruby @kiranktp @anchu-rajendran @raghavendhra @ronlieb >> If you have ideas on other improvements of the process, I'm happy to try >> them out. > > Let people know that you changed your mind before they put in the time and > effort. I am sure that is not a big ask. I believe I did let everyone know as early as I knew. I'm unsure how I should improve on this. I mention my thoughts in reviews and I usually include a ping to relevant @people. I also assume (or hope) that interested parties watch phabricator (or the mailing list), e.g., for OpenMP patches, so they stay informed about what is happening. > --- > > Anyways, I suggested something that you didn't reply to, which you may have > missed. To resolve this, would you be willing to go for: > > 1. You handle any typing problems with this patch when you commit it and > D79676 <https://reviews.llvm.org/D79676> after D80222 > <https://reviews.llvm.org/D80222> > 2. I bring back all my runtime def.s that I need, and use macros per your > original suggestion, and you commit this and D79676 > <https://reviews.llvm.org/D79676> today or tomorrow and that patch can merge > based on head commit which it will do anyway? I'm not even sure I follow. D80222 <https://reviews.llvm.org/D80222> landed, as far as I can tell. Can you elaborate on these suggestions so I do not misinterpret them? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
