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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
