[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D79675#2051682 <https://reviews.llvm.org/D79675#2051682>, @fghanim wrote:

> > My goal is to save us time, during development, review, maintenance, and future extensions. I hope you know that.
>
> I am certain of that. However, I am starting to have doubts if my time is accounted for as part of "save us time".


That is unfortunate because it is. Even if you don't believe me, it seems illogical for me to waste your time on purpose assuming I benefit from the work, wouldn't you agree?

Since I can neither change what I've said before, nor predict how future changes impact my past comments, I will not argue with you on me changing my mind. 
That happens even without outside interference during a code review.

In this particular situation I suggested something and someone came up with something better in a different patch, I will not defend something for the sake of having suggested
it in the first place. Instead, I made you aware of it and asked if there is a reason to not adopt the better solution we haven't considered before. This is the same as with code.
We replace code from anyone with something better as it becomes available. That is a good thing, not something to argue about.

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.

---

I'm sorry you feel I waste your time. I really would not do so on purpose. In order to avoid such situations we should have quick reviews.
I already try to provide feedback as soon as I can. While more reviewers would obviously help, it is known that smaller patches do too.
If you have ideas on other improvements of the process, I'm happy to try them out.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+  /// return a copy of the current insertion point information
+  InsertPointTy SaveIP() { return Builder.saveIP(); }
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79675