jvoung wrote:
> It would be better to use `GtestCmp` if you really want clang-tidy support
> gtest in this check. Here is an unfinished example which can match
>
> ```
> TEST(a, b) {
> std::optional<int> a;
> if (v) {
> a = 1;
> }
> ASSERT_NE(a, std::nullopt);
> a.value();
> }
> ```
>
> ```c++
> using namespace ast_matchers;
> auto HasOptionalCallDescendant =
> hasDescendant(callExpr(callee(cxxMethodDecl(
> ofClass(UncheckedOptionalAccessModel::optionalClassDecl())))));
> Finder->addMatcher(
> decl(anyOf(functionDecl(
> unless(isExpansionInSystemHeader()),
> // FIXME: Remove the filter below when lambdas are
> // well supported by the check.
> unless(hasDeclContext(cxxRecordDecl(isLambda()))),
> hasBody(allOf(
> HasOptionalCallDescendant,
> unless(hasDescendant(gtestAssert(
> GtestCmp::Ne,
>
> expr(hasType(qualType(hasUnqualifiedDesugaredType(
> recordType(hasDeclaration(
> UncheckedOptionalAccessModel::
> optionalClassDecl())))))),
> anything())))))),
> cxxConstructorDecl(hasAnyConstructorInitializer(
> withInitializer(HasOptionalCallDescendant)))))
> .bind(FuncID),
> this);
> ```
Sorry I missed this comment earlier!
I think doing "unless( ... gtestAssert... GtestCmp...)" for filtering this way
would be more complex for not much gain, over the current filtering. Is the
concern accuracy?
regarding complexity: there are many forms to check if the optional has a
value, so doing filters this way (with "unless") we would need to enumerate all
of them (gtestExpect, gtestExpectThat, gtestAssertThat, a to-be-added
gtestExpectTrue, gtestAssertTrue, might need GtestCmp::Eq as well as Ne, like
ASSERT_EQ(a.has_value(), true)). Similarly if we want to filter out catch2.
At that point we may as well handle the matcher + transfer functions to "see
through" the macros in the checker itself rather than filtering here in a
simple way (but then see prioritization concern).
regarding accuracy: I've run a large scale experiment across our monorepo, and
the current method seems accurate w.r.t. filtering test files, test utilities,
test base classes, fakes, mock classes, etc. Only 0.4% of the diffs didn't have
"test/Test/TEST/..." etc in the path for file name (as a rough check). Among
that 0.4% some really were test-only libraries that were less obviously
test-only from the file name (golden_generator, sampler, fake, mock, benchmark,
etc.), and some were timeout diffs.
https://github.com/llvm/llvm-project/pull/115051
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
