[PATCH] D111288: [fir] Add data flow optimization pass

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/Transforms/MemRefDataFlowOpt.cpp:29
+  std::vector<OpT> ops;
+  for (auto *user : v.getUsers())
+    if (auto op = dyn_cast<OpT>(user))
----------------
clementval wrote:
> clementval wrote:
> > mehdi_amini wrote:
> > > clementval wrote:
> > > > mehdi_amini wrote:
> > > > > mehdi_amini wrote:
> > > > > > 
> > > > > This is marked as done, but it isn't (it isn't the only one I think)
> > > > I have discussed this with others and they would like to keep it like that. The guidlines are kind of blurry on this... If this is a blocker we will ave to discuss this further. 
> > > Let's discuss then.
> > > (and please don't ignore comments or mark them as done when they aren't!)
> > Sorry for that. We have discussed in this patch or in maybe another one and in the last call that during the upstreaming work any "blocker" comment should be put as "Change Requested" so that we can identified where to put our effort. 
> > In this particular case `auto` can surely be removed but not everywhere like you mentioned in your comment. I think that's more the everywhere that others were against. 
> > 
> > > `auto` does not help readability so why not spell out the type? (here and everywhere, in all the upstreaming patches please).
> On this particular comment, I remember what happened. I did the change and marked it as done then after discussion I revrted the change but I forgot to mention that. Sorry for that, I didn't mean to discard your comment. 
@mehdi_amini We had discussion with the team and they mentioned couple of points why they would like to keep `auto`: 
- It avoids to introduce type's bugs like
`auto ch = getc(file);`
changed to 
`char ch = getc(file);`
it's a small type change. The char type looks like documentation but it is actually introducing a bug that might be really hard to detect. 
- Also refactoring is easier if we have type inference.
Not sure if this is addressing your comment here. If not please let us know and we can discuss further how we can best approach this since it will be a recurrent pattern in the Flang code. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111288