sammccall added a comment. In D83508#2155335 <https://reviews.llvm.org/D83508#2155335>, @ArcsinX wrote:
> In D83508#2155322 <https://reviews.llvm.org/D83508#2155322>, @sammccall wrote: > > > In D83508#2155174 <https://reviews.llvm.org/D83508#2155174>, @ArcsinX wrote: > > > > > In D83508#2143625 <https://reviews.llvm.org/D83508#2143625>, @sammccall > > > wrote: > > > > > > > Your test cases show two problems with this strategy: > > > > > > > > - we ignore comments and semicolons, but not preprocessor directives > > > > (or disabled preprocessor regions). I think we can fix this by asking > > > > TokenBuffer if a spelled token is part of a region that maps to no > > > > (PP-)expanded tokens. > > > > > > > > > I have tried this locally. seems it breaks SelectionTest.IncludedFile > > > test. > > > > > > Yeah that makes sense, I guess it just says nothing is selected in that > > case? > > > Yes, and test crashes at `T.commonAncestor()` (which is `nullptr`) > dereference. So can we also add `ASSERT_TRUE(T.commonAncestor());` into > several tests? Right, that particular case should become EXPECT_EQ(..., nullptr). Technically the other tests should probably have an ASSERT, because otherwise if the code is broken and returns nullptr we have UB and the test could spuriously pass. In practice I've never seen a test spuriously pass this way, let alone on all buildbots. And these kinds of assumptions are hard to keep out of the tests. So I'm not particularly concerned about it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83508/new/ https://reviews.llvm.org/D83508 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
