I'm sorry for the delay with review. In general the patch looks good to me.
- Please cleanup the patch and remove unnecessary files from it.
- Fix issues with `-std=c++11` flag and `LLVM_OVERRIDE` macro. See inline
comments below.
- Ask Rafael Espindola or Chandler Carruth about review. The changes are
rather significant and I would like to see more LGTM.
================
Comment at: include/clang/Driver/Multilib.h:42
@@ +41,3 @@
+ assert(GCCSuffix.empty() ||
+ (GCCSuffix.front() == '/' && GCCSuffix.size() > 1));
+ return GCCSuffix;
----------------
g++ shows an error on this line. It does not know about std::string::front()
method if you do not specify -std=c++11 flag. Have you built the clang by g++
and using autotools configure script or CMake with default options?
================
Comment at: include/clang/Driver/Multilib.h:52
@@ +51,3 @@
+ assert(OSSuffix.empty() ||
+ (OSSuffix.front() == '/' && OSSuffix.size() > 1));
+ return OSSuffix;
----------------
Ditto
================
Comment at: include/clang/Driver/Multilib.h:62
@@ +61,3 @@
+ assert(IncludeSuffix.empty() ||
+ (IncludeSuffix.front() == '/' && IncludeSuffix.size() > 1));
+ return IncludeSuffix;
----------------
Ditto
================
Comment at: include/clang/Driver/Multilib.h:74
@@ +73,3 @@
+ Multilib &flag(std::string F) {
+ assert(F.front() == '+' || F.front() == '-');
+ Flags.push_back(F);
----------------
Ditto
================
Comment at: lib/Driver/Multilib.cpp:335
@@ +334,3 @@
+ FilterWrapper(const MultilibSet::FilterCallback &F) : F(F) {}
+ bool operator()(const Multilib &M) const LLVM_OVERRIDE { return F(M); }
+};
----------------
Why do you need `LLVM_OVERRIDE` here?
http://llvm-reviews.chandlerc.com/D2538
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
