[PATCH] D61827: [clang-tidy] modernize-loop-convert: impl const cast iter

[Roman Lebedev via Phabricator via cfe-commits]

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