DavidSpickett added a comment. > Context not available.
Please update the diff with context, https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. I'm not sure what the benefit of this change is. While I did say less raw pointers = better, perhaps I'd make an exception for `const char *` for constant strings. In this case, no one is manipulating the string until (I assume) after it's converted to std::string, so we're not removing risky accesses in that way. If the API were returning a `const char *` that we then did a bunch of find first, split at, strlen, etc on, then this would make more sense. https://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes Also this says you can implicitly construct from a string literal, so I am surprised you need the `llvm::StringLiteral`. Unless this is just a thing that asserts that it is in fact, a literal (I've not used this before myself). Overall I applaud the effort but in this particular case it may not be needed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148799/new/ https://reviews.llvm.org/D148799 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
