[PATCH] D134058: [lld-macho] Support -dyld_env

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lld/MachO/Driver.cpp:1414-1415
+  for (auto *arg : args.filtered(OPT_dyld_env)) {
+    const char *envPair = arg->getValue();
+    if (!strchr(envPair, '='))
+      error("-dyld_env malformed value. Expected "
----------------
total nit but I think it's clearer this way. I don't actually know off the top of my head what `strchr` returns in error cases (though I can certainly infer based on how it's used here)


================
Comment at: lld/MachO/Driver.cpp:1416
+    if (!strchr(envPair, '='))
+      error("-dyld_env malformed value. Expected "
+            "-dyld_env <ENV_VAR>=<VALUE>, got `" +
----------------
grammar nit: `malformed -dyld_env value` or `-dyld_env's argument is malformed`


================
Comment at: lld/MachO/Driver.cpp:1418
+            "-dyld_env <ENV_VAR>=<VALUE>, got `" +
+            Twine(envPair) + "`");
+    config->dyldEnvs.push_back(envPair);
----------------
if we construct `envPair` as a StringRef then I don't think we'll need the `Twine()` here


================
Comment at: lld/MachO/Options.td:1215-1220
+    
+def dyld_env : Separate<["-"], "dyld_env">,
+    MetaVarName<"<dyld_env_var>">,
+    HelpText<"Specifies a LC_DYLD_ENVIRONMENT variable value pair.">,
+    Group<grp_rare>;
+    
----------------
can we place this along with the rest of the `grp_rare` options? No need for the leading/trailing newlines too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134058