================
Comment at: lib/Sema/SemaStmt.cpp:2412
@@ +2411,3 @@
+ E = MTE->GetTemporaryExpr();
+ } else {
+ llvm_unreachable("Unexpected expression encountered.");
----------------
dblaikie wrote:
> Avoid branch-to-unreachable, just uncoditionally
> cast<MaterializeTemporaryExpr> in the previous block (as an else, not an
> else-if)
Done.
================
Comment at: lib/Sema/SemaStmt.cpp:2427
@@ +2426,3 @@
+ } else {
+ llvm_unreachable("Unexpected expression encountered.");
+ }
----------------
dblaikie wrote:
> 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)
Done.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:13
@@ +12,3 @@
+template <typename T>
+class Container {
+ typedef Iterator<T&> I;
----------------
dblaikie wrote:
> 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.
Done.
================
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{{}}
----------------
dblaikie wrote:
> Is it much hassle to just have the code work in such a way that it doesn't
> have errors?
Yes, we could remove all the non-const reference bindings that fail. Each test
tries to bind/assign a type 4 different ways, T, T&, const T, and const T&.
Since the other type is convertible, binding to T& sometimes will fail. I
think we should keep them for completeness.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:23
@@ +22,3 @@
+template <typename T>
+class ContainerNonReference {
+ typedef Iterator<T> I;
----------------
dblaikie wrote:
> 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.
Done.
================
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}}
+
----------------
dblaikie wrote:
> 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)
I don't recall how we handle removing references in other cases. Here, the
code author had marked to loop variable const, making the intent clear that the
const should be kept.
================
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'}}
----------------
dblaikie wrote:
> 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?
The @ after the expected diagnostic can work both ways, using minus to refer to
earlier lines and plus to refer to later lines. In grep, I found 130 files
with @-NUM while there were 120 files with @+num.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:180
@@ +179,3 @@
+ for (int x : C) {}
+ // Now warning, copy made
+}
----------------
dblaikie wrote:
> typo, 'no' rather than 'now'?
Fixed.
================
Comment at: test/SemaCXX/warn-range-loop-analysis.cpp:292
@@ +291,3 @@
+void test9() {
+ Bar I[2] = {1,2};
+
----------------
dblaikie wrote:
> 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)
The arrays use a pointer type as their iterator. The Container struct above
used a user-defined iterator. The code path is slightly different for the two.
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
