[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

[Diana Picus via Phabricator via cfe-commits]

rovka added a comment.

IMO this is a reasonable approach, I only have a few nits.



================
Comment at: flang/runtime/environment.cpp:42
+#else
+    if (setenv(name, value, 0) == -1) {
+#endif
----------------



================
Comment at: flang/runtime/environment.cpp:77
+#else
+  envp = environ;
+#endif
----------------
peixin wrote:
> jpenix-quic wrote:
> > peixin wrote:
> > > What is `environ` used for? Should we try to keep as less extern variable as possible in runtime for security.
> > I think `setenv` is only required to make sure that the `environ` pointer points to the correct copy of the environment variables. So, as long as we are storing a copy of the environment pointer in `ExecutionEnvironment` and potentially making changes to the environment via `setenv`, I think we need to base it off the `environ` pointer after `setenv` has been called rather than the `envp` from `main`.
> > 
> > That said, I don't think the `envp` variable we are storing is being used for anything at the moment (reading environment variables was changed to use `getenv` rather than `envp` in the commit [[ https://github.com/llvm/llvm-project/commit/824bf908194c9267f1f09065ba8e41d7969006ab | here]]). If removing the usage of `environ` is preferable, we could maybe remove the usage of `envp` altogether (in a separate patch)--does this sound like a good idea or would it be better to just leave in the `environ` usage for now?
> Thanks for the explanations. The current fix makes sense to me.
I agree that we should remove envp altogether in a follow-up patch. As it stands it's just causing confusion.


================
Comment at: flang/test/Driver/convert.f90:1
+! Ensure argument -fconvert=<value> works as expected.
+
----------------
Nit: Shouldn't you also check that valid values (e.g. unknown, swap etc) are accepted? 


================
Comment at: flang/test/Driver/driver-help.f90:27
 ! HELP-NEXT: -fcolor-diagnostics    Enable colors in diagnostics
+! HELP-NEXT: -fconvert=<value>       Set endian conversion of data for unformatted files
 ! HELP-NEXT: -fdefault-double-8     Set the default double precision kind to an 8 byte wide type
----------------
Nit: Why is there an extra blank for these?


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

https://reviews.llvm.org/D130513