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

[Pete Steinfeld via Phabricator via flang-commits]

PeteSteinfeld requested changes to this revision.
PeteSteinfeld added a comment.
This revision now requires changes to proceed.

I have some questions ...



================
Comment at: flang/include/flang/Lower/SymbolMap.h:351-352
+
+  llvm::Optional<fir::FortranVariableOpInterface>
+  lookupVariableDefinition(semantics::SymbolRef sym);
+
----------------
This function is not used anywhere.  Will future changes will make use of it?


================
Comment at: flang/lib/Lower/ConvertVariable.cpp:1342
                              bool force = false) {
+
+  if (converter.getLoweringOptions().getLowerToHighLevelFIR()) {
----------------
Did you mean for this blank line to be here?


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


================
Comment at: flang/lib/Lower/SymbolMap.cpp:103-106
+        return {};
+    }
+  }
+  return {};
----------------
Should these be returning `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";
----------------
Why does one of these terminate in a newline and the other doesn't?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136252