[PATCH] D78835: [flang] Upstream recent work on FIR to llvm-project.

Tue Apr 28 02:07:15 PDT 2020

DavidTruby added a comment.

In D78835#2006859 <https://reviews.llvm.org/D78835#2006859>, @mehdi_amini wrote:

> In D78835#2005053 <https://reviews.llvm.org/D78835#2005053>, @DavidTruby wrote:
>
> > Would it be possible to update the commit message/summary to describe what's changed here?
>
>
> I think it'd be great to keep the commit messages in the repo actually being descriptive: "Upstream recent work on FIR to llvm-project" does not tell us anything about the commit, compared to for example something like "Add MemoryEffects to FIR operations".
>
> It also seems like this is mixing a bunch of different thing that really should be different commits instead (That may contributes to the difficulty to provide a clear commit message).

I completely agree with Mehdi here; a link to an external repo that has a proper commit history isn't a substitute for a meaningful commit message in our repo. A commit message like "Upstream recent work on FIR to llvm-project" tells me nothing about what the commit does if I end up needing to look back through the history/bisect a bug etc. It's also not sufficient to say "this is a set of commits that has been reviewed externally and that I'm upstreaming as one" because the community on the linked repository is different to (and much smaller than) the community of people that participate in Flang in LLVM.

It is usually polite to wait for someone who has requested changes to approve before committing; this doesn't seem to have happened with @rriddle's comments here. Especially since one of your replies to their comments was that you were unsure of something I think it would have been a lot better to give them time to clarify.
I also would have expected to be given time to reply on my request before this was committed, as I had requested something specific and would expect to at least be able to rebut a reply dismissing my concerns.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78835