flx added a comment. In https://reviews.llvm.org/D26195#585917, @aaron.ballman wrote:
> In https://reviews.llvm.org/D26195#585198, @flx wrote: > > > In https://reviews.llvm.org/D26195#585091, @aaron.ballman wrote: > > > > > In https://reviews.llvm.org/D26195#584958, @flx wrote: > > > > > > > In https://reviews.llvm.org/D26195#584730, @aaron.ballman wrote: > > > > > > > > > In https://reviews.llvm.org/D26195#584724, @flx wrote: > > > > > > > > > > > In https://reviews.llvm.org/D26195#584712, @aaron.ballman wrote: > > > > > > > > > > > > > Please add a test case with an incomplete type that would > > > > > > > exercise this code path, otherwise, LGTM. > > > > > > > > > > > > > > > > > > Hi Aaron, > > > > > > > > > > > > do you have any advise on how to add an incomplete type? When > > > > > > debugging this I had a compilation unit that failed to compile > > > > > > causing it, but I'm not sure this is a good way to add a test case. > > > > > > > > > > > > > > > A type like `class C;` is an incomplete type, as is `void`, so > > > > > perhaps you can find a check that would let such a construct call > > > > > through to `isExpensiveToCopy()`. > > > > > > > > > > > > Great, this works and I was able to see the check produce a false > > > > positive without the proposed change here, but the test code introduces > > > > a compile error now due to the incomplete type used in the function > > > > definition. Is there a way to suppress that? > > > > > > > > > Unlikely -- fixing the compile error likely makes the type not expensive > > > to copy by using a pointer (or reference). This may be tricky to test > > > because the times when you would call `isExpensiveToCopy()` is with types > > > that are going to be logically required to be complete. I am not certain > > > the compile error is actually a problem though -- I would imagine your > > > existing false-positives (that you mentioned in the patch summary) are > > > cases where there is a compile error *and* a clang-tidy diagnostic, so > > > the test may simply be "check that there's only a compile error and no > > > clang-tidy diagnostic where there used to be a false-positive one." > > > > > > That's exactly the case, my question here is how can I make the test > > succeed in the face of a compile error, i.e. by expecting the error as well? > > > Doesn't `// CHECK-MESSAGES: :[[@LINE-1]]:3: error: blah blah blah` work? Yes, that was needed. I also had to change the run command to add "-fix-errors" to still apply fixes in the face of compile errors. I also added you as reviewer, Aaron. https://reviews.llvm.org/D26195 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
