mgehre marked 2 inline comments as done. mgehre added inline comments.
================ Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" ---------------- thakis wrote: > mgehre wrote: > > thakis wrote: > > > This weird relative include path is a hint that this isn't the intended > > > use :) > > True. But ... > > I thought about copying the `matches` function from STMatchersTest.h, but > > together with all callees, that's many lines of code. > > Alternatively, I could implement my own AST walking and property checking, > > but then the test would be much less readable. > > As yet another alternative, I though about moving the > > `GslOwnerPointerInference.cpp` test itself to `unittests/ASTMatchers`, > > but that would also be strange because it actually tests Sema functionality. > > > > What would be your suggestion? (It's good that we discuss this now because > > I'm planning to extend this unit test further). > Do you need the full matches() function? With the current test, it's not even > clear to me what exactly it tests since it looks very integration-testy and > not very unit-testy. Maybe if you make the test narrower, you don't that much > scaffolding? (Integration-type tests are supposed to be lit tests, and unit > tests are supposed to test small details that are difficult to test via lit.) The purpose of the test is to verify that the ClassTemplateSpecialization gets the Owner attribute, independent of which of the redeclarations of the template had the attribute attached. The second thing that I want to test here in the future is that the inference for std:: types leads to correct Owner/Pointer attributes on the `ClassTemplateSpecialization`., i.e. a type named `std::vector` should have OwnerAttr on each of its `ClassTemplateSpecialization`. We currently do this in a lit test here https://reviews.llvm.org/differential/changeset/?ref=1574879 via checking the AST dump, but that is really hard to debug in cases of failures, and not entirely clear that it checks exactly what we want. (E.g. we can check that a `ClassTemplateSpecializationDecl` line is followed by a `PointerAttr` line, but we don't actually know if that attribute hierarchically belongs to that decl or a another one). So @gribozavr suggested to me to instead break this integration tests into smaller chunks using the ASTMatchers, which at least solves that problem. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66179/new/ https://reviews.llvm.org/D66179 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
