On 22/06/2014 18:37, David Blaikie wrote:
================
Comment at: lib/Sema/SemaStmt.cpp:2361
@@ +2360,3 @@
+ const CXXForRangeStmt *ForStmt) {
+ if (SemaRef.Diags.getDiagnosticLevel(
+ diag::warn_for_range_const_reference_copy, ForStmt->getLocStart()) ==
----------------
I think Alp's been trying to move to using an "isIgnored" function to do this
test for some reason - I haven't looked into the motivation, but it might be something to
check if you should be consistent with.
Yes, parsing and semantic analysis shouldn't be concerned with
diagnostic levels other than opportunistically checking if the
diagnostic is definitely ignored.
This separation of concerns means it'll become possible to support
interactive post-compile diagnostic mapping which is a great timesaver
for IDE and analysis tool workflows.
(In the meantime, it also lets us validate that enabling warning flags
doesn't affect code generation or AST output -- unfortunately this does
happen in places.)
Alp.
================
Comment at: lib/Sema/SemaStmt.cpp:2373
@@ +2372,3 @@
+
+ const VarDecl *VD = ForStmt->getLoopVariable();
+ if (!VD)
----------------
What are the cases where a for loop has no loop variable? (or the loop variable
has no init, down on line 2382)
================
Comment at: lib/Sema/SemaStmt.cpp:2386
@@ +2385,3 @@
+
+ if (VariableType->isReferenceType()) {
+ // Foo foos[5];
----------------
Might be worth splitting out these cases as separate functions, maybe?
================
Comment at: lib/Sema/SemaStmt.cpp:2449
@@ +2448,3 @@
+ // Foo foos[5];
+ // for (const Bar bar : foos);
+ bool MakeCopy = false;
----------------
The header comment for this function doesn't mention this case of Foo v Bar, it
just mentions:
Foo foos[5];
for (const Foo f : foos);
->
for (const Foo &f : foos);
And the implementation looks like that's the intent of the code. Is the comment
out of date/wrong?
================
Comment at: lib/Sema/SemaStmt.cpp:2452
@@ +2451,3 @@
+ if (const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(InitExpr)) {
+ if (CE->getConstructor()->isCopyConstructor())
+ MakeCopy = true;
----------------
Not sure if it'd be more readable, but you could just assign:
MakeCopy = CE->getConstructor()->isCopyConstructor();
================
Comment at: lib/Sema/SemaStmt.cpp:2455
@@ +2454,3 @@
+ } else if (const CastExpr *CE = dyn_cast<CastExpr>(InitExpr)) {
+ if (CE->getCastKind() == CK_LValueToRValue)
+ MakeCopy = true;
----------------
Same assignment possible here
================
Comment at: lib/Sema/SemaStmt.cpp:2467
@@ +2466,3 @@
+ }
+ return;
+ }
----------------
return here seems a bit unnecessary (and/or as above, this whole case might be
nice as a separate function for readability)
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:14
@@ +13,3 @@
+class Container {
+ typedef Iterator<return_type> I;
+
----------------
I'm not sure if this merits a change to the test code, but this is a bit
surprising/out-of-character to have a container of T return T by value from its
iterator's op*. (usually it would return a reference to T)
================
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'}}
----------------
I think this is going to have a pretty high false positive rate... do you have
some numbers?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:46
@@ +45,3 @@
+
+void test1() {
+ typedef Container<int> non_reference_iterator_container;
----------------
If these test functions provide logical grouping, perhaps they could be named
based on that grouping, or have a comment describing it?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:47
@@ +46,3 @@
+void test1() {
+ typedef Container<int> non_reference_iterator_container;
+ non_reference_iterator_container A;
----------------
any reason for the typedef here, but not in test0 above?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:50
@@ +49,3 @@
+
+ for (const int &x : A) {}
+ // expected-warning@-1 {{always a copy}}
----------------
Looks the same as the previous tests? (line 33, specifically)
If this test is to ensure that a container wrapped in a typedef is handled
correctly - what kind of problems did you have with this situation that you had
to handle/correct for?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:73
@@ +72,3 @@
+ // expected-note@-2 {{'const Bar'}}
+ for (const Bar x : A) {}
+ // No warning, non-reference type indicates copy is made
----------------
I worry about people creating expensive copies this way, but maybe that'll need
to be a clang-tidy check rather than a warning.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:83
@@ +82,3 @@
+ typedef Container<int&> reference_iterator_container;
+ reference_iterator_container B;
+
----------------
Same questions as above
http://reviews.llvm.org/D4169
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
