dblaikie added inline comments.
================ Comment at: clang/test/CodeGen/unique-internal-linkage-names.cpp:48-49 +// LPIPELINE: Unique Internal Linkage Names +// NPIPELINE: Running pass: UniqueInternalLinkageNamesPass // PLAIN: @_ZL4glob = internal global ---------------- Does this test validate the new behavior? (ie: does this test fail without the LLVM changes and pass with it) Not that it necessarily has to - since Clang isn't here to test the LLVM behavior - perhaps this test is sufficient in Clang to test that the code in BackendUtil works to enable this pass. This could possibly be staged as independent commits - adding the LLVM functionality in one commit, which would be a no-op for Clang because it wouldn't be setting PTO.UniqueLinkageNames - then committing the Clang change that would remove the custom pass addition and set PTO.UniqueLinkageNames - and then it'd probably be reasonable to have this test be made a bit more explicit (testing the pass manager structure/order) to show that that Clang change had an effect: Moving the pass to the desired location in the pass pipeline. ================ Comment at: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll:1-2 +; RUN: opt -S -unique-internal-linkage-names < %s | FileCheck %s +; RUN: opt -S -passes=unique-internal-linkage-names < %s | FileCheck %s + ---------------- Does this test test any changes that are made in the rest of the patch? Since the test is specifying the pass to test, I would've assumed this test would pass with or without any changes that might move that pass around in the pass order (or indeed add or remove it from the pass manager in general). I'd expect this change to be tested in LLVM by a pass manager structure test, similar to the Clang test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93656/new/ https://reviews.llvm.org/D93656 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
