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

[Diana Picus via Phabricator via flang-commits]

rovka added a comment.

In D111785#3075853 <https://reviews.llvm.org/D111785#3075853>, @klausler wrote:

> Another thing that you should consider: if the program adds or modifies its environment via the POSIX functions setenv(3) and unsetenv(3), your code may not notice the changes unless it uses std::getenv() exclusively -- the captured envp pointer may not reflect those changes and may not even still be valid.

Yeah, to be honest I'm a bit confused about the existence of envp in the first place. Why exactly are we storing it? I was thinking it would be kind of a "quick" path, so we could skip allocating a C-style string, but if there's a possibility for envp to become stale then maybe we shouldn't have it at all? Or is the idea that when we do have an envp, then the environment cannot be modified and envp has the final word?



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


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

But then we won't have operator[].

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

Will do.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111785