[flang-commits] [PATCH] D111785: [flang] runtime: Read environment variables directly

[Peter Klausler via Phabricator via flang-commits]

klausler added inline comments.


================
Comment at: flang/include/flang/Runtime/memory.h:39
 
+template <typename A> struct OwningPtrDeleter<A[]> {
+  void operator()(A *p) { FreeMemory(p); }
----------------
I'm not sure that this specialization is necessary.  Does the code compile without it?


================
Comment at: flang/runtime/environment.cpp:98
+
+static OwningPtr<char[]> NullTerminatedString(
+    const char *name, std::size_t name_length) {
----------------
`char` can be used here  instead of `char]` and would be more idiomatic.

This function might have use cases elsewhere; consider exposing it in tools.h or memory.h.


================
Comment at: flang/runtime/environment.cpp:116
+    return value;
+  }
+
----------------
If GetEnvFromEnvp returns a null pointer, how do you know that it was due to a null envp pointer rather that an absent environment variable setting for the name?  In the latter case, you should return a null pointer here as well, yes?


================
Comment at: flang/runtime/environment.cpp:122
+  OwningPtr<char[]> cStyleName{NullTerminatedString(name, name_length)};
+  if (!cStyleName) {
+    std::fputs("Fortran runtime: out of memory for env var name\n", stderr);
----------------
I think that it would be okay to crash when out of memory here.  A null return from GetEnv() should signify only that the name was not found.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111785