================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:41
@@ +40,3 @@
+
+ for (const int x : int_ref_container) {}
+ // expected-warning@-1 {{loop variable 'x' of type 'const int' creates a
copy from type 'int'}}
----------------
Richard Trieu wrote:
> David Blaikie wrote:
> > Richard Trieu wrote:
> > > David Blaikie wrote:
> > > > I think this is going to have a pretty high false positive rate... do
> > > > you have some numbers?
> > > What is your idea of a false positive?
> > Users are taught to pass ints to functions by value, rather than reference,
> > to avoid the indirection overhead. The same logic applies (to a degree) to
> > for loops. It's not unreasonable to write "for (const int i :
> > my_vec_of_ints)".
> >
> > I don't think this case is likely to be a good thing to warn on and we may
> > need a heuristic of "if a type is 'large' enough (or has a non-trivial
> > copy), warn when a copy is made". StringRef would be a similar situation -
> > I can imagine lots of people (I bet we've got a few in Clang/LLVM already)
> > looping over StringRef's by value.
> >
> > Do you have some stats on running this warning in its current form over
> > LLVM/Clang? (and Google, for that matter, or any other substantial
> > codebase).
> >
> > Looking at other test cases, this wouldn't warn if the "const" was removed?
> > Perhaps we just need a fix to disregard cv qualifiers, then?
> LLVM/Clang has a total of 5 warnings emitted for this warning. (Where
> changing the variable to reference type will prevent a copy.) Two are pairs
> from maps, one is a CXXBaseSpecifier, one is QualType, and one is a
> ParmVarDecl pointer. From your description, the QualType and Decl pointer
> are both small and trivially copyable so they should be exempt from this
> warning, so a 40% drop in cases with your suggested filter.
>
> Applying the same filter to results from Google, the drop is closer to 70%.
> The filter is to ignore trivially copyable types which are 2x pointer size or
> smaller.
>
> The const is a good indication that the variable is read-only, so we can
> suggest using a const-reference as the cheaper option. Without the const, we
> don't know if the use in the loop was intended only for the loop copy or to
> change the object in the container as well.
You mention a 70% drop in Google - but what does a random sample of the
remaining (and omitted) cases look like?
I'm trying to understand if this warning is good or not, as-is, which means we
need a reasonable false positive % estimate/number to decide whether it needs
to be tweaked further to reduce false positives (that's the main thing - if
it's got lots of false negatives that we can gain back, that can be done in
separate patches, but going in-tree with high false positive rates is generally
frowned upon)
Perhaps I'm missing something where you described this already.
http://reviews.llvm.org/D4169
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
