dexonsmith accepted this revision.
dexonsmith added a comment.
LGTM too, once the test is fixed.
================
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"))));
----------------
Bigcheese wrote:
> 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.
Agreed, should be string comparison. IMO passing `StringRef("...")` into
`count` is clear / easy enough, but a custom matcher would be fine too.
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
