george.burgess.iv added a comment.
Thanks for the review!
Repository:
rL LLVM
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL293360: Change how we handle diagnose_if attributes.
(authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D28889?vs=86146&id=86155#toc
Repository:
rL LLVM
https://reviews.llvm.org/D
george.burgess.iv added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:11933
}
-
aaron.ballman wrote:
> Unintended change?
...I dunno what keeps making this change, but I'll re-undo it before I submit.
https://reviews.llvm.org/D28889
___
george.burgess.iv added inline comments.
Comment at: test/SemaCXX/diagnose_if.cpp:615
+// evaluator isn't able to evaluate `adl::Foo(1)` to a constexpr, though.
+// I'm assuming this is because we assign it to a temporary.
+for (void *p : adl::Foo(1)) {}
-
george.burgess.iv updated this revision to Diff 86146.
george.burgess.iv marked 7 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback
> Another "fun" testcase
Ooh, shiny. Added.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/cla
rsmith added a comment.
Another "fun" testcase:
struct S {
void operator++(int n) _diagnose_if(n, "wat", "warning");
};
void f(S s) {
s++; // no warning
s.operator++(1); // warning
}
Comment at: include/clang/Sema/Sema.h:2638
- /// Check the diagnose_if
george.burgess.iv updated this revision to Diff 86111.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Address feedback
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
lib/Sem
george.burgess.iv added a comment.
Thanks for the feedback!
Comment at: lib/Sema/SemaChecking.cpp:2520
+
+// TODO: Call can technically be a const CallExpr, but const_casting feels
ugly,
+// and I really don't want to duplicate unwrapCallExpr's logic. No caller
really
---
aaron.ballman added a comment.
I don't see anything that looks amiss, but you should wait for @rsmith to
approve.
Comment at: lib/Sema/SemaChecking.cpp:2520
+
+// TODO: Call can technically be a const CallExpr, but const_casting feels
ugly,
+// and I really don't want to dupl
george.burgess.iv added a comment.
Pinging early because this is a release blocker. :)
https://reviews.llvm.org/D28889
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> Also, I plan to submit this (once it's LGTM'ed) to the 4.0 branch. Is
that OK with you, Richard?
(To be clear, I'll check with Hans before I submit this there, as well.
Just trying to save a round-trip. :) )
On Mon, Jan 23, 2017 at 4:39 PM, George Burgess IV via Phabricator <
revi...@reviews.ll
george.burgess.iv updated this revision to Diff 85486.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Addressed all feedback.
Richard noted that, because we're now doing these checks after overload
resolution has occurred, we no longer need to convert argu
rsmith added inline comments.
Comment at: include/clang/Sema/Overload.h:758
/// instead.
+/// FIXME: Now that it only alloates ImplicitConversionSequences, do we
want
+/// to un-generalize this?
Typo "alloates"
Comment at: lib/Sem
george.burgess.iv updated this revision to Diff 84988.
george.burgess.iv added a comment.
Sprinkle in a few `const`s, use `const auto *` in range for loops.
https://reviews.llvm.org/D28889
Files:
include/clang/Sema/Overload.h
include/clang/Sema/Sema.h
lib/Sema/SemaChecking.cpp
lib/Sema/
george.burgess.iv created this revision.
As it turns out, emitting diagnostics from places where you're not meant to
emit them from is a very bad idea. :)
After some looking around, it seems that it's less insane to check for
`diagnose_if` attributes in code that's already checking for e.g. nul
15 matches
Mail list logo
