[PATCH] Add Multilib selection machinery to Clang

Simon Atanasyan simon at atanasyan.com
Wed Jan 29 02:09:29 PST 2014


  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



More information about the cfe-commits mailing list