[flang-commits] [PATCH] D136252: [flang] optionally lower scalar and explicit shape with fir.declare

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks @PeteSteinfeld



================
Comment at: flang/include/flang/Lower/SymbolMap.h:351-352
+
+  llvm::Optional<fir::FortranVariableOpInterface>
+  lookupVariableDefinition(semantics::SymbolRef sym);
+
----------------
PeteSteinfeld wrote:
> This function is not used anywhere.  Will future changes will make use of it?
Soon, but you are right, I'll just remove it until it is used.


================
Comment at: flang/lib/Lower/ConvertVariable.cpp:1367-1369
   if (converter.getLoweringOptions().getLowerToHighLevelFIR())
     TODO(genLocation(converter, sym),
          "generate fir.declare when lowering symbol");
----------------
PeteSteinfeld wrote:
> It doesn't look like this code will ever be reached.
Indeed, removed.


================
Comment at: flang/lib/Lower/SymbolMap.cpp:103-106
+        return {};
+    }
+  }
+  return {};
----------------
PeteSteinfeld wrote:
> Should these be returning `llvm::None`?
I think it is equivalent. I removed this code anyway for now. I'll try to remember adding it back with llvm::None :).


================
Comment at: flang/lib/Lower/SymbolMap.cpp:129-132
+    return os << *symBox;
+  auto definingOp =
+      std::get<fir::FortranVariableOpInterface>(symboxOrdefiningOp);
+  return os << definingOp << "\n";
----------------
PeteSteinfeld wrote:
> Why does one of these terminate in a newline and the other doesn't?
> Why does one of these terminate in a newline and the other doesn't?

It is a bit confusing, but `os << *symBox;` calls  `Fortran::lower::operator<<(llvm::raw_ostream &os, const Fortran::lower::SymbolBox &symBox)` define just above that adds new lines on its own. The SymBox part should anyway be removed when the transition is over. So in both cases a new line is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136252