[PATCH] [libcxx] Add compiler utility class for LIT tests
eric at efcs.ca
Sat Jan 17 00:04:25 PST 2015
Address @danalbert's comments.
Comment at: test/libcxx/test/compiler.py:5
@@ +4,3 @@
+ def __init__(self, path, flags=, compile_flags=, link_flags=, use_ccache=False):
> 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)
> 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:
+ compiler_type = None
> 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]
> 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
> `macro, value = l.split()`
`value` can contain spaces. We only want to split on the first space, not all of them.
More information about the cfe-commits