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