[PATCH] D27686: Add llvm::StringLiteral

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/ADT/StringRef.h:90
-        assert((data || length == 0) &&
-        "StringRef cannot be built from a NULL argument with non-null length");
-      }
----------------
Too bad we're losing the assert, but I see why. An alternative is another (possibly protected) ctor that would be constexpr and used by `StringLiteral`


================
Comment at: include/llvm/ADT/StringRef.h:839
 
+  /// StringLiteral - A wrapper around a StringLiteral that serves as a proxy
+  /// for constructing global tables of StringRefs with the length computed
----------------
zturner wrote:
> yaron.keren wrote:
> > wrapper around StringRef
> I'm open to better wording here, but I don't think this is accurate either.  A "wrapper around a StringRef" implies that you need a `StringRef` in order to create one of these.  But it's exactly the opposite, you really do need a string literal (i.e. object of the form "abcdefg") to create one of these.
Nit, from the coding standards: `Don’t duplicate function or class name at the beginning of the comment. For humans it is obvious which function or class is being documented; automatic documentation processing tools are smart enough to bind the comment to the correct declaration.`

(Also technically it is more of a `wrapper around a string literal` more than around a StringLiteral) 


================
Comment at: include/llvm/ADT/StringRef.h:843
+  /// considered undefined behavior.  To prevent this, it is recommended
+  /// that StringLiteral *only* be used in a constexpr context, as such:
+  ///
----------------
May add a warning about null in the middle of a literal? (That's a behavior change from StringRef)


https://reviews.llvm.org/D27686