<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jan 26, 2014 at 9:46 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">They say you should practice what you preach, and this commit had some changes that'll affect all users of StringRef so I've rolled it back in r200187 until we get some discussion going.<br>
<br>
The need derives from dozens of cases in clang where user input was being passed in as a format string to the diagnostic system. With all those fixed following ongoing work the last few months we now want a stricter check for compile-time fixed strings and this general solution has come together.<br>
<br>
I'll kick off with a self-review, posted inline below.. :-)<div><div class="h5"><br>
<br>
<br>
On 27/01/2014 04:07, Alp Toker wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: alp<br>
Date: Sun Jan 26 22:07:17 2014<br>
New Revision: 200187<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=200187&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=200187&view=rev</a><br>
Log:<br>
StringRef: Extend constexpr capabilities and introduce ConstStringRef<br>
<br>
(1) Add llvm_expect(), an asserting macro that can be evaluated as a constexpr<br>
expression as well as a runtime assert or compiler hint in release builds. This<br>
technique can be used to construct functions that are both unevaluated and<br>
compiled depending on usage.<br>
<br>
(2) Update StringRef using llvm_expect() to preserve runtime assertions while<br>
extending the same checks to static asserts in C++11 builds that support the<br>
feature.<br>
<br>
(3) Introduce ConstStringRef, a strong subclass of StringRef that references<br>
compile-time constant strings. It's convertible to, but not from, ordinary<br>
StringRef and thus can be used to add compile-time safety to various interfaces<br>
in LLVM and clang that only accept fixed inputs such as diagnostic format<br>
strings that tend to get misused.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/<u></u>StringRef.h<br>
llvm/trunk/include/llvm/<u></u>Support/ErrorHandling.h<br>
llvm/trunk/lib/Support/<u></u>ErrorHandling.cpp<br>
llvm/trunk/unittests/ADT/<u></u>StringRefTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/<u></u>StringRef.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringRef.h?rev=200187&r1=200186&r2=200187&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/include/<u></u>llvm/ADT/StringRef.h?rev=<u></u>200187&r1=200186&r2=200187&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/include/llvm/ADT/<u></u>StringRef.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/<u></u>StringRef.h Sun Jan 26 22:07:17 2014<br>
@@ -10,6 +10,8 @@<br>
#ifndef LLVM_ADT_STRINGREF_H<br>
#define LLVM_ADT_STRINGREF_H<br>
+#include "llvm/Support/Compiler.h"<br>
+#include "llvm/Support/ErrorHandling.h"<br>
#include "llvm/Support/type_traits.h"<br>
#include <algorithm><br>
#include <cassert><br>
@@ -70,7 +72,7 @@ namespace llvm {<br>
/// @{<br>
/// Construct an empty string ref.<br>
- /*implicit*/ StringRef() : Data(0), Length(0) {}<br>
+ /*implicit*/ LLVM_CONSTEXPR StringRef() : Data(0), Length(0) {}<br>
/// Construct a string ref from a cstring.<br>
/*implicit*/ StringRef(const char *Str)<br>
@@ -80,11 +82,8 @@ namespace llvm {<br>
}<br>
/// Construct a string ref from a pointer and length.<br>
- /*implicit*/ StringRef(const char *data, size_t length)<br>
- : Data(data), Length(length) {<br>
- assert((data || length == 0) &&<br>
- "StringRef cannot be built from a NULL argument with non-null length");<br>
- }<br>
+ /*implicit*/ LLVM_CONSTEXPR StringRef(const char *data, size_t length)<br>
+ : Data(data), Length((llvm_expect(data || length == 0), length)) {}<br>
</blockquote>
<br></div></div>
We lose friendly messages here, but we get back subjectively equal benefit by using the built-in LLVM fatal error handler instead of the one in the system.<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
/// Construct a string ref from an std::string.<br>
/*implicit*/ StringRef(const std::string &Str)<br>
@@ -104,24 +103,20 @@ namespace llvm {<br>
/// data - Get a pointer to the start of the string (which may not be null<br>
/// terminated).<br>
- const char *data() const { return Data; }<br>
+ LLVM_CONSTEXPR const char *data() const { return Data; }<br>
/// empty - Check if the string is empty.<br>
- bool empty() const { return Length == 0; }<br>
+ LLVM_CONSTEXPR bool empty() const { return Length == 0; }<br>
/// size - Get the string size.<br>
- size_t size() const { return Length; }<br>
+ LLVM_CONSTEXPR size_t size() const { return Length; }<br>
/// front - Get the first character in the string.<br>
- char front() const {<br>
- assert(!empty());<br>
- return Data[0];<br>
- }<br>
+ LLVM_CONSTEXPR char front() const { return llvm_expect(!empty()), Data[0]; }<br>
/// back - Get the last character in the string.<br>
- char back() const {<br>
- assert(!empty());<br>
- return Data[Length-1];<br>
+ LLVM_CONSTEXPR char back() const {<br>
+ return llvm_expect(!empty()), Data[Length - 1];<br>
}<br>
/// equals - Check for string equality, this is more efficient than<br>
@@ -187,9 +182,8 @@ namespace llvm {<br>
/// @name Operator Overloads<br>
/// @{<br>
- char operator[](size_t Index) const {<br>
- assert(Index < Length && "Invalid index!");<br>
- return Data[Index];<br>
+ LLVM_CONSTEXPR char operator[](size_t Index) const {<br>
+ return llvm_expect(Index < Length), Data[Index];<br>
}<br>
</blockquote>
<br></div></div>
Ditto.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
/// @}<br>
@@ -547,6 +541,20 @@ namespace llvm {<br>
/// @}<br>
+ /// ConstStringRef - A \c StringRef carrying the additional stipulation that<br>
+ /// the referenced string is a compile-time constant.<br>
+ ///<br>
+ /// Use this to specify function parameters that require fixed inputs such<br>
+ /// as debug and diagnostic messages or format strings.<br>
+ class ConstStringRef : public StringRef {<br>
+ public:<br>
+ /*implicit*/ LLVM_CONSTEXPR ConstStringRef() : StringRef() {}<br>
+<br>
+ template <size_t N><br>
+ /*implicit*/ LLVM_CONSTEXPR ConstStringRef(const char (&data)[N])<br>
+ : StringRef(data, (llvm_expect(N > 0 && data[N - 1] == '\0'), N - 1)) {}<br>
</blockquote>
<br></div>
This constructor doesn't technically require a constant string literal and you can trick it into accepting a fixed-length char array, but in all uses it detected errors that have been fixed. </blockquote><div><br></div>
<div>Could you point to revisions that demonstrate the fixes that have been prompted by this?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">There's some interest in introducing a clang attribute to check for literals and that would get added here as the feature evolves.<div>
<div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ };<br>
+<br>
/// \brief Compute a hash_code for a StringRef.<br>
hash_code hash_value(StringRef S);<br>
<br>
Modified: llvm/trunk/include/llvm/<u></u>Support/ErrorHandling.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/ErrorHandling.h?rev=200187&r1=200186&r2=200187&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/include/<u></u>llvm/Support/ErrorHandling.h?<u></u>rev=200187&r1=200186&r2=<u></u>200187&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/include/llvm/<u></u>Support/ErrorHandling.h (original)<br>
+++ llvm/trunk/include/llvm/<u></u>Support/ErrorHandling.h Sun Jan 26 22:07:17 2014<br>
@@ -15,11 +15,11 @@<br>
#ifndef LLVM_SUPPORT_ERRORHANDLING_H<br>
#define LLVM_SUPPORT_ERRORHANDLING_H<br>
-#include "llvm/ADT/StringRef.h"<br>
#include "llvm/Support/Compiler.h"<br>
#include <string><br>
namespace llvm {<br>
+ class StringRef;<br>
class Twine;<br>
/// An error handler callback.<br>
@@ -78,7 +78,7 @@ namespace llvm {<br>
bool gen_crash_diag = true);<br>
LLVM_ATTRIBUTE_NORETURN void report_fatal_error(const std::string &reason,<br>
bool gen_crash_diag = true);<br>
- LLVM_ATTRIBUTE_NORETURN void report_fatal_error(StringRef reason,<br>
+ LLVM_ATTRIBUTE_NORETURN void report_fatal_error(const StringRef &reason,<br>
bool gen_crash_diag = true);<br>
</blockquote>
<br></div></div>
This tweak drops the early dependency on StringRef.h so we can use ErrorHandling in StringRef itself.<div class="im"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
LLVM_ATTRIBUTE_NORETURN void report_fatal_error(const Twine &reason,<br>
bool gen_crash_diag = true);<br>
@@ -108,4 +108,14 @@ namespace llvm {<br>
#define llvm_unreachable(msg) ::llvm::llvm_unreachable_<u></u>internal()<br>
#endif<br>
+/// An assert macro that's usable in constexprs and that becomes an optimizer<br>
+/// hint in NDEBUG builds.<br>
+///<br>
+/// Unlike \c assert() the \param test expression may be evaluated in optimized<br>
+/// builds and so should be simple, accurate and never have side effects.<br>
+#define llvm_expect(test) (void)(!!(test) ? 0 : (llvm_unreachable(#test), 0))<br>
</blockquote>
<br></div>
This macro expansion could do with checking over. If it works out llvm_expect() will have better characteristics in debug builds.<br>
<br>
The optimizer hint approach is something to consider. We have to evaluate 'test' to support constexpr evaluation but we don't strictly need to use it as a hint if llvm_unreachable() feels too strong here.<br>
<br>
Alp.<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+// TODO: Update other headers to explicitly include StringRef.h and drop this.<br>
+#include "llvm/ADT/StringRef.h"<br>
+<br>
#endif<br>
<br>
Modified: llvm/trunk/lib/Support/<u></u>ErrorHandling.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/ErrorHandling.cpp?rev=200187&r1=200186&r2=200187&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>Support/ErrorHandling.cpp?rev=<u></u>200187&r1=200186&r2=200187&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Support/<u></u>ErrorHandling.cpp (original)<br>
+++ llvm/trunk/lib/Support/<u></u>ErrorHandling.cpp Sun Jan 26 22:07:17 2014<br>
@@ -58,7 +58,7 @@ void llvm::report_fatal_error(const std:<br>
report_fatal_error(Twine(<u></u>Reason), GenCrashDiag);<br>
}<br>
-void llvm::report_fatal_error(<u></u>StringRef Reason, bool GenCrashDiag) {<br>
+void llvm::report_fatal_error(const StringRef &Reason, bool GenCrashDiag) {<br>
report_fatal_error(Twine(<u></u>Reason), GenCrashDiag);<br>
}<br>
<br>
Modified: llvm/trunk/unittests/ADT/<u></u>StringRefTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringRefTest.cpp?rev=200187&r1=200186&r2=200187&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/unittests/<u></u>ADT/StringRefTest.cpp?rev=<u></u>200187&r1=200186&r2=200187&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/unittests/ADT/<u></u>StringRefTest.cpp (original)<br>
+++ llvm/trunk/unittests/ADT/<u></u>StringRefTest.cpp Sun Jan 26 22:07:17 2014<br>
@@ -531,4 +531,22 @@ TEST(StringRefTest, joinStrings) {<br>
EXPECT_TRUE(v2_join3);<br>
}<br>
+static void fn_stringref(StringRef str) {<br>
+ EXPECT_TRUE(str == "hello");<br>
+}<br>
+static void fn_conststringref(<u></u>ConstStringRef str) {<br>
+ fn_stringref(str);<br>
+}<br>
+<br>
+TEST(StringRefTest, constStringRef) {<br>
+ LLVM_CONSTEXPR ConstStringRef csr("hello");<br>
+#if __has_feature(cxx_constexpr) || defined(__GXX_EXPERIMENTAL_<u></u>CXX0X__)<br>
+ LLVM_STATIC_ASSERT(csr[0] != csr[1], "");<br>
+ LLVM_STATIC_ASSERT(csr[2] == csr[3], "");<br>
+ LLVM_STATIC_ASSERT(csr.size() == 5, "");<br>
+#endif<br>
+ llvm_expect(csr[2] == csr[3]);<br>
+ fn_conststringref(csr);<br>
+}<br>
+<br>
} // end anonymous namespace<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br></div></div><span class="HOEnZb"><font color="#888888">
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>