fghanim marked an inline comment as done. fghanim added a comment. 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. > 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 > 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? > 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. --- 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? ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62 + /// return a copy of the current insertion point information + InsertPointTy SaveIP() { return Builder.saveIP(); } + ---------------- jdoerfert wrote: > fghanim wrote: > > jdoerfert wrote: > > > Unused? > > I'll happily drop them if you want. I needed them at one point, and assumed > > we may need them later, and left them in to see what you think. So still > > LGTM , or no LGTM? > Generally we should not include code we don't need (or that is not tested). We don't need it at the moment, however, I do not think an IRBuilder should go without a way to specify where you want it to point at. This doesn't need a test. it just passes an argument to a private `IRBuilder` - if that works, this should just work. Bottom line, should I remove it? 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
