Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803613003:
"Escape" certain characters (Closed)
Description"Escape" certain characters
Added EscapeString() to convert certain characters into XML escape (&xxx;) sequences
BUG=708705
Review-Url: https://codereview.chromium.org/2803613003
Cr-Commit-Position: refs/heads/master@{#462966}
Committed: https://chromium.googlesource.com/chromium/src/+/7dbbea4c0d8e6f34a12ab7ce3b4a92d36e25d2f8
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address feeback. Run git cl format. #Messages
Total messages: 15 (6 generated)
Description was changed from ========== "Escape" certain characters Added EscapeString() to convert certain characters into XML escape (&xxx;) sequences BUG=708705 ========== to ========== "Escape" certain characters Added EscapeString() to convert certain characters into XML escape (&xxx;) sequences BUG=708705 ==========
kylixrd@chromium.org changed reviewers: + brettw@chromium.org, brucedawson@chromium.org
This issue came up recently when I noticed that many of the generated vcxproj files would fail to load in Visual Studio when pre-processor definitions contain "<" or "<" characters such as the "<<" and ">>" operators. Other characters that may appear there should also be escaped such as '"'. The problem has been "fixed" by changing the pre-processor definitions to not use the "<<" and ">>" operators. That only fixes the symptom and not the actual problem.
https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:47: case '\n': result += " "; break; Do you have references for the \n, \r, and \t sequences? This document suggests that there are only five legal escape sequences in XML: http://stackoverflow.com/questions/1091945/what-characters-do-i-need-to-escap... Speaking of which, you might as well support '.
https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:47: case '\n': result += " "; break; On 2017/04/05 18:51:39, brucedawson wrote: > Do you have references for the \n, \r, and \t sequences? This document suggests > that there are only five legal escape sequences in XML: > > http://stackoverflow.com/questions/1091945/what-characters-do-i-need-to-escap... > > Speaking of which, you might as well support '. I based it off the code in libxml here: https://cs.chromium.org/chromium/src/third_party/libxml/src/xmlsave.c?l=2049
lgtm
Per the request (plz ping after 24h), consider this the "ping" :)
LGTM with some updates. https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:42: if (value.find_first_of("<>&\"\t\r\n") != std::string::npos) { I'd delete this check and always do the loop below. find_first_of will basically do the same thing your loop+switch does and you code copies the strings in all cases anyway, so this extra check is just extra code and runtime work. https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:43: for (std::string::const_iterator it = value.cbegin(); I would have used a range-based for loop here which is a lot easier to read: for (char c : value) { switch (c) { ... } https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:58: } else Google style says all arms in an if statement should match so this should have a { } (although I suggest deleting it above so this is just an FYI).
https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... File tools/gn/visual_studio_writer.cc (right): https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:42: if (value.find_first_of("<>&\"\t\r\n") != std::string::npos) { On 2017/04/07 18:16:43, brettw (plz ping after 24h) wrote: > I'd delete this check and always do the loop below. find_first_of will basically > do the same thing your loop+switch does and you code copies the strings in all > cases anyway, so this extra check is just extra code and runtime work. Done. https://codereview.chromium.org/2803613003/diff/1/tools/gn/visual_studio_writ... tools/gn/visual_studio_writer.cc:43: for (std::string::const_iterator it = value.cbegin(); On 2017/04/07 18:16:43, brettw (plz ping after 24h) wrote: > I would have used a range-based for loop here which is a lot easier to read: > > for (char c : value) { > switch (c) { > ... > } Yes, that's better. Done.
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2803613003/#ps20001 (title: "Address feeback. Run git cl format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491592749525300,
"parent_rev": "51cdcbaa84f6221a9b9b2957bbb5f750e6414c1b", "commit_rev":
"7dbbea4c0d8e6f34a12ab7ce3b4a92d36e25d2f8"}
Message was sent while issue was closed.
Description was changed from ========== "Escape" certain characters Added EscapeString() to convert certain characters into XML escape (&xxx;) sequences BUG=708705 ========== to ========== "Escape" certain characters Added EscapeString() to convert certain characters into XML escape (&xxx;) sequences BUG=708705 Review-Url: https://codereview.chromium.org/2803613003 Cr-Commit-Position: refs/heads/master@{#462966} Committed: https://chromium.googlesource.com/chromium/src/+/7dbbea4c0d8e6f34a12ab7ce3b4a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7dbbea4c0d8e6f34a12ab7ce3b4a... |
