kristina added a comment. I'll re-review when I'm up, from a quick glance it looks much better but I'll have to patch it over my fork and try out a few things (Mostly x86_64 Linux and Darwin test suites). I think the test is lacking a bit, there's a lot of stuff that isn't covered, and there's a lack of negative tests (ie. when invalid input is supplied and matches the triple suffix). Feel free to run tests though, may find something before me, I'm a bit too tired to reconfigure my buildbot to do what I want right now, so I'll leave it until when I'm up. So yeah I may be being a bit nitpicky but I think tests could cover a little bit more.
It would also be great to get at least one other Clang reviewer to sign off on this. I can sign off on this myself once I test it, but I feel like getting another set of eyes would be good. But yeah if this is all good and someone else can also skim through this and sign off on it, I can commit the stack when I'm up and when I've done some stuff. If you can, try to build/test with a recent Clang as that usually brings out some benign warnings one may miss if using an older SDK. But very good job in general, this seems a lot better and streamlined than the previous revision. So that said to other Clang reviewers: Gentle ping, would really love a second set of eyes though, though it can wait until Monday at worst since I'd imagine a lot of people are off right now. Thank you. Repository: rC Clang https://reviews.llvm.org/D54379 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
