lebedev.ri 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()))), > Yes and no. This **will** prevent the false-positive, but **only if** the OpenMP is **enabled** (`-fopenmp`). If OpenMP is **not enabled**, then that **won't work**, because there won't be anything about OpenMP in AST. I semi-strongly believe it will be less confusing to only document this false-positive (in check's docs), instead of trying to prevent it, and reliably failing in half of the cases. 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
