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

[Diana Picus via Phabricator via flang-commits]

rovka added a comment.

I've put ERRMSG handling in a different patch to make it easier to review, but we can also squash it into this one.



================
Comment at: flang/runtime/command.cpp:60
+  if (n < 0 || n >= executionEnvironment.argc) {
+    if (IsValidCharDescriptor(value)) {
+      FillWithSpaces(value);
----------------
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?


================
Comment at: flang/runtime/command.cpp:72
+    }
+    if (argLen >= static_cast<LengthType>(value->ElementBytes())) {
+      return -1;
----------------
It's not very clear to me from the standard what's supposed to happen to 'value' if it's too short. I guess it could be helpful to write as much of the argument as we can fit into it, but then we'd also have to decide whether we should put a \0 at the end or not (*), and overall I don't know if it's worth the trouble. Thoughts?

(*) IIUC gfortran doesn't force a \0, which makes the code easier but the testing harder, because I think it's UB to call EXPECT_STREQ on a char string that's not null-terminated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109813