[PATCH] [libcxx] Add compiler utility class for LIT tests

[Eric Fiselier]

Address @danalbert's comments.


================
Comment at: test/libcxx/test/compiler.py:5
@@ +4,3 @@
+
+class CXXCompiler(object):
+    def __init__(self, path, flags=[], compile_flags=[], link_flags=[], use_ccache=False):
----------------
danalbert wrote:
> I probably would have put this directly in the `libcxx` module rather than `libcxx.test`, but nbd.
I think that's a good change.

================
Comment at: test/libcxx/test/compiler.py:13
@@ +12,3 @@
+        self.type = None
+        self.version = (None, None, None)
+        self._initTypeAndVersion()
----------------
danalbert wrote:
> This should maybe be just `None`, since you want it to abort if try to use it before it's initialized (and `bool((None, None, None)) == True`).
Oop, didn't think of that. Thanks.

================
Comment at: test/libcxx/test/compiler.py:20
@@ +19,3 @@
+        if macros is None:
+            return
+        compiler_type = None
----------------
danalbert wrote:
> Isn't this an error? Should probably raise an exception...
Perhaps `dumpMacros()` should throw an exception if the return code is non-zero, but I don't think it makes sense for `_initTypeAndVersion()` to throw because then `CXXCompiler` wouldn't work with compilers that didn't support the dump macro command.

Any suggestion on the type of the exception?

================
Comment at: test/libcxx/test/compiler.py:48
@@ +47,3 @@
+            cmd += infiles
+        elif isinstance(infiles, str):
+            cmd += [infiles]
----------------
danalbert wrote:
> I'm not a fan of using one argument to accept multiple types that don't conform to the same interface (or behave like the same duck, as it were). Just require that `infiles` is a `list`. The argument name is plural, after all.
I'm not a big fan either but I think its the best option. I really hate forcing the transformation from string -> list at every call site, I think that makes the the calling code less clear. Even if we only accept a list of files we still have to diagnose being passed a string. Requiring a list is almost the same amount of work as just accepting it.

If this were C++ I would almost certainly have two overloads for both a single file and for a list of files. Accepting both types is quite useful for the user and I don't believe it makes the code unclear.

What problems do you envision this code causing?

================
Comment at: test/libcxx/test/compiler.py:107
@@ +106,3 @@
+            l = l[len('#define '):]
+            macro, _, value = l.partition(' ')
+            parsed_macros[macro] = value
----------------
danalbert wrote:
> `macro, value = l.split()`
`value` can contain spaces. We only want to split on the first space, not all of them.

http://reviews.llvm.org/D7019

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/