[flang-commits] [PATCH] D109813: [flang] GET_COMMAND_ARGUMENT(VALUE) runtime implementation

[Peter Klausler via Phabricator via flang-commits]

klausler added inline comments.


================
Comment at: flang/runtime/command.cpp:60
+  if (n < 0 || n >= executionEnvironment.argc) {
+    if (IsValidCharDescriptor(value)) {
+      FillWithSpaces(value);
----------------
rovka wrote:
> klausler wrote:
> > rovka wrote:
> > > We're silently ignoring 'value' if it's not allocated or not the right type - should we have an assert instead if it doesn't look the way we expect?
> > The `value` argument should be a reference, not a pointer, to a descriptor here (and in your IsValidCharDescriptor() utility above).  It is not optional if ArgumentValue() is actually being called.
> Actually I was thinking it is optional. For instance a call to GET_COMMAND_ARGUMENT(NUMBER, LENGTH, STATUS, ERRMSG) would be lowered to a call to ArgumentLength() and one to ArgumentValue() without a descriptor for value, and the latter would just fill in the STATUS and ERRMSG. I guess lowering could skip the call to ArgumentValue and fill in STATUS and ERRMSG based on the return from ArgumentLength, but wouldn't that just be more complicated than necessary?
Ok, SGTM.  Please clean up the types and I'll approve.


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

https://reviews.llvm.org/D109813