[PATCH] D78051: Introduce `EnvArray` data structure.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

To my taste it's too complicated for task it's going to solve.



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_env_array.h:53
+// allocator and is not very sophisticated.
+template <uptr STATIC_ENV_SLOTS = 64, uptr STATIC_STRING_STORAGE_SIZE = 64>
+class EnvArray {
----------------
I would prefer if we avoid parameters which can be avoided.
How user should decide which size to use?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_env_array.h:106
+  uptr copyFromRawEnv(char** envp, bool shallow_copy,
+                      bool allow_heap_allocations, bool overwrite) {
+    CHECK(isValid());
----------------
It can be much simpler without 
"bool shallow_copy, bool allow_heap_allocations, bool overwrite"


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_env_array.h:144
+
+  bool envArrayOnHeap() const { return envp_storage_heap_.Size() > 0; }
+
----------------
As I understand the goal of the class to manage copy of env variables.
However public interface expose to much details about internal storage.
Could you please limit public interface as much as possible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78051