lebedev.ri added a comment. In D61827#1499333 <https://reviews.llvm.org/D61827#1499333>, @hintonda wrote:
> In D61827#1499309 <https://reviews.llvm.org/D61827#1499309>, @lebedev.ri > wrote: > > > In D61827#1499306 <https://reviews.llvm.org/D61827#1499306>, @hintonda > > wrote: > > > > > 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, > > > > > > How so? > > > When I asked for a test above, I understood you to say you couldn't provide > one, but If I misunderstood, by all means, please add the test. Please do note that i have provided a testcase (godbolt link) in my very first comment, and quoted that line when replying the previous time. (Granted, that loop is not in a correct form for openmp, but the point being, the current check does not diagnose it either) 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
