thakis added inline comments.
================ Comment at: cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp:9 + +#include "../ASTMatchers/ASTMatchersTest.h" +#include "clang/ASTMatchers/ASTMatchers.h" ---------------- 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.) 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
