hintonda added a comment. In D61827#1499303 <https://reviews.llvm.org/D61827#1499303>, @torbjoernk wrote:
> In D61827#1499184 <https://reviews.llvm.org/D61827#1499184>, @lebedev.ri > wrote: > > > In D61827#1499183 <https://reviews.llvm.org/D61827#1499183>, @hintonda > > wrote: > > > > > In D61827#1499160 <https://reviews.llvm.org/D61827#1499160>, @lebedev.ri > > > wrote: > > > > > > > This will now trigger on https://godbolt.org/z/9oFMcB right? > > > > Just want to point out that this will then have "false-positives" when > > > > that loop > > > > is an OpenMP for loop, since range-for loop is not available until > > > > OpenMP 5. > > > > > > > > I don't think this false-positive can be avoided though, if building > > > > without > > > > `-fopenmp` there won't be anything about OpenMP in AST, > > > > and thus no way to detect this case.. > > > > > > > > > Could you suggest a simple test case that could be added to the test? > > > That way, instead of just removing the `if else` block, @torbjoernk could > > > try to handle it. Or perhaps exclude it from the match altogether. > > > > > > As i said, i don't see how this can be avoided in general. > > > I have to admit that I have very little experience with OpenMP and haven't > thought of this at all. Thank you very much for bringing this up. > > Would it help to extend the exclusion AST matcher for iterator-based loops by > an exclusion for loops with an ancestor of `ompExecutableDirective`?: > > return forStmt( > unless(anyOf(isInTemplateInstantiation(), > hasAncestor(ompExecutableDirective()))), > As a general rule, don't add anything that doesn't include a test. Since this "false positive" is apparently untestable, as far as I'm concerned, it doesn't exist. Most tests of this sort are mocked up to emulate the problem so you can verify it exists and the fix actually addresses it. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61827/new/ https://reviews.llvm.org/D61827 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
