DavidSpickett added inline comments.

================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1422
+    def runBuildCommands(self, commands):
+        for cmd in commands:
+            self.trace(shlex.join(cmd))
----------------
Is this ever going to have more than one command to run?

Seems like the source is the function above that puts a single command into a 
list.


================
Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1284
         compiler = self.getCompilerBinary()
-        version_output = system([[compiler, "--version"]])
-        for line in version_output.split(os.linesep):
-            m = re.search('version ([0-9.]+)', line)
-            if m:
-                return m.group(1)
+        version_output = check_output([compiler, "--version"])
+        m = re.search(b'version ([0-9.]+)', version_output)
----------------
labath wrote:
> DavidSpickett wrote:
> > You could use `universal_newlines` here to get the decoded string. It's a 
> > bit cryptic but saves the decode below.
> > 
> > There is an alias `text` name in 3.7 but requiring that seems ambitious.
> how about `error="replace"` on the `check_output` invocation it's equally 
> cryptic, and does not throw an exception on non-utf output?
> 
> (I'm generally unsure about the best way to handle the byte-string duality in 
> python3 -- whether to try to handle things at the byte level (if they can) or 
> to convert everything to strings as soon as possible and pretend bytes don't 
> exist.)
> and does not throw an exception on non-utf output

I didn't consider that so yeah this looks fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112212

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to