[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#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?

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?
> > > >
> > >
> >
>




> 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