compnerd added inline comments.
================ Comment at: clang/lib/Driver/Driver.cpp:3372 + if (Phase == phases::IfsMerge) { + assert(Phase == PL.back() && "merging must be final compilation step."); + MergerInputs.push_back(Current); ---------------- Does the interface merging have to be the last step? I could see interface merging preceding linking just fine. ================ Comment at: clang/lib/Driver/Driver.cpp:3468 + case phases::IfsMerge: llvm_unreachable("link action invalid here."); case phases::Preprocess: { ---------------- Please update the unreachable message ================ Comment at: clang/test/InterfaceStubs/driver-test.cpp:17 +// CHECK-DAG: _Z8weakFuncv + +int foo(int bar) { ---------------- Should we not ensure that no other symbols are exported? ================ Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:3 + +// -x c so that we dont have name-mangling. +// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so -emit-interface-stubs \ ---------------- Why not name the file `merge-conflict-test.c`? ================ Comment at: clang/test/InterfaceStubs/merge-conflict-test.cpp:4 +// -x c so that we dont have name-mangling. +// RUN: not %clang -target x86_64-linux-gnu -x c -o libfoo.so -emit-interface-stubs \ +// RUN: %s %S/driver-test.cpp 2>&1 | FileCheck %s ---------------- Why not leave the `-target` off entirely? That will allow you to drop the restriction of the `x86-registered-target` ================ Comment at: clang/test/InterfaceStubs/object-double.cpp:2 +// REQUIRES: x86-registered-target +// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s %S/object.cpp 2>&1 | \ +// RUN: FileCheck %s ---------------- Similar ================ Comment at: clang/test/InterfaceStubs/object-float.cpp:2 +// REQUIRES: x86-registered-target +// RUN: not %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s %S/object.cpp 2>&1 | \ +// RUN: FileCheck %s ---------------- And here ================ Comment at: clang/test/InterfaceStubs/object.cpp:5 -// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \ -// RUN: -interface-stub-version=experimental-ifs-v1 %s | \ +// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s | \ // RUN: FileCheck -check-prefix=CHECK-SYMBOLS %s ---------------- Might as well as do it here as well ================ Comment at: clang/test/InterfaceStubs/object.ifs:1 +# RUN: %clang -emit-merged-ifs -target x86_64-unknown-linux-gnu \ +# RUN: -emit-interface-stubs -o - %s | \ ---------------- Can we drop the target and get this to be portable? ================ Comment at: clang/test/InterfaceStubs/template-namespace-function.cpp:2 // REQUIRES: x86-registered-target -// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \ -// RUN: -interface-stub-version=experimental-ifs-v1 %s | \ +// RUN: %clang -c -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs %s | \ // RUN: FileCheck %s ---------------- Try to drop the x86 requirement throughout Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63978/new/ https://reviews.llvm.org/D63978 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits