================
Comment at: lib/Sema/SemaStmt.cpp:2412
@@ +2411,3 @@
+ E = MTE->GetTemporaryExpr();
+ } else {
+ llvm_unreachable("Unexpected expression encountered.");
----------------
Avoid branch-to-unreachable, just uncoditionally cast<MaterializeTemporaryExpr>
in the previous block (as an else, not an else-if)
================
Comment at: lib/Sema/SemaStmt.cpp:2427
@@ +2426,3 @@
+ } else {
+ llvm_unreachable("Unexpected expression encountered.");
+ }
----------------
Prefer an assertion rather than branch-to-unreachable
(eg: change the else if to an else, then just use cast<CXXOperatorCallExpr>
since it must be true)
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:13
@@ +12,3 @@
+template <typename T>
+class Container {
+ typedef Iterator<T&> I;
----------------
Maybe just have one Container class that takes the iterator element type as a
parameter? (T, T&, etc) - essentially your ContainerNonReference (renamed), and
just instantiate it with T& if you want the reference version.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:17
@@ +16,3 @@
+public:
+ // These notes are from errors, which this test doesn't care much about.
+ I begin(); // expected-note 4{{}}
----------------
Is it much hassle to just have the code work in such a way that it doesn't have
errors?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:23
@@ +22,3 @@
+template <typename T>
+class ContainerNonReference {
+ typedef Iterator<T> I;
----------------
I'd probably jsut make all these classes structs & skip the access modifiers -
there's not much to hide/fence off in a test case.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:87
@@ +86,3 @@
+ // expected-warning@-1 {{loop variable 'x' has type 'const double &' but is
initialized with type 'int' resulting in a copy}}
+ // expected-note@-2 {{use non-reference type 'const double' to keep the copy
or type 'const int &' to prevent copying}}
+
----------------
We seem to have some varied opinions on whether to drop 'const' when removing a
reference - is their diagnostic precedence in clang we can draw from where we
already suggest dropping refs (to see if we also suggest dropping const there
too)?
I seem to recall this coming up in some discussion about a clang-tidy fix
recently (the pass-by-value fixit hint, which is very similar to this warning)
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:98
@@ +97,3 @@
+ for (const int &x : A) {}
+ // expected-warning@-1 {{always a copy}}
+ // expected-note@-2 {{'const int'}}
----------------
Having the comments /after/ the code in question is a bit hard to read - is
that intentional? Could it be the other way around, with the comment coming
before the code?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:180
@@ +179,3 @@
+ for (int x : C) {}
+ // Now warning, copy made
+}
----------------
typo, 'no' rather than 'now'?
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:292
@@ +291,3 @@
+void test9() {
+ Bar I[2] = {1,2};
+
----------------
Is the array case in any way differently handled in the code itself? If not, I
probably wouldn't bother testing them (here, or the previous 2 test functions)
http://reviews.llvm.org/D4169
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
