[Lldb-commits] [PATCH] D74579: [don't review]Creating environment variable test for lldbd

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/environmentVariables/TestVSCode_environmentVariables.py:31-35
+        found = False
+        for line in lines:
+            if line.startswith('PATH='):
+                found = True
+                self.assertTrue(path_env_variable == line, "PATH environment variable not the same")
----------------
this becomes more readable if you make it a function. That way you avoid having the mutable found variable and you can stop the for loop as soon as you return


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/environmentVariables/main.cpp:10
+    char** env_var_pointer = environ;
+    // std::vector<std::string> vscode_env_variables;
+    int count = 0;
----------------
remove this comment


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py:315
         self.assertTrue(os.path.exists(program), 'executable must exist')
-
         self.launch(program, args, cwd, env, stopOnEntry, disableASLR,
----------------
try not to delete existing blank lines


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:905
                                       stopCommands=options.stopCmds,
-                                      exitCommands=options.exitCmds)
+                                      exitCommands=options.exitCmds
+                                      inheritEnvironment=options.inheritEnvironment
----------------
I think you need a comma here


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:906-907
+                                      exitCommands=options.exitCmds
+                                      inheritEnvironment=options.inheritEnvironment
+                                      )
 
----------------
put the ) in line 906


================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1408-1410
+  if (!envs.empty()) {
     g_vsc.launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
+  }
----------------
in llvm, you don't need to use { } for one line ifs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74579