Bigcheese accepted this revision. Bigcheese added a comment. This revision is now accepted and ready to land.
lgtm with the test fix. ================ Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:267 - ASSERT_THAT(GeneratedArgs, Contains(StrEq("-fdebug-pass-manager"))); + ASSERT_EQ(count(GeneratedArgs, "-fdebug-pass-manager"), 1); ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-fno-debug-pass-manager")))); ---------------- jansvoboda11 wrote: > dexonsmith wrote: > > jansvoboda11 wrote: > > > Is it wise to rely on pointer comparison here? The call to `count` > > > returns 2 before changing the denormalizer and 1 after, but I'm not sure > > > if it will work on all platforms. > > Does this compile / avoid the pointer comparison? > > ``` > > ASSERT_EQ(count(GeneratedArgs, StringRef("-fdebug-pass-manager")), 1); > > ``` > Yes, this forces the `const char *` from GeneratedArgs to be converted into > `StringRef`, which does comparison via `::memcmp`. > > If we decide comparing `StringRef`s is a better solution, I'd be inclined to > create a helper function or custom matcher that does the conversion to > `StringRef`. That way, we don't have to worry about doing the right thing in > test cases. I believe that if you have shared libraries enabled this is guaranteed to be different. We should do a string comparison here, and I think a helper makes sense if we expect to do this more often. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93094/new/ https://reviews.llvm.org/D93094 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits