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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
