[PATCH] D46579: [llvm-rc] Handle C preprocessor output

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:22
+public:
+  Filter(StringRef Input) : Data(Input), DataLength(Input.size()) {}
+
----------------
Mark with `explicit`.


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:34
+  StringRef Data;
+  size_t DataLength, Pos;
+
----------------
Can you put these on different lines?  Also, `Pos` and `Outputting` are uninitialized member variables, can we initialize them inline?


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:63
+
+  if (Line.size() == 0 || Line[0] != '#') {
+    // A normal content line, filtered according to the current mode.
----------------
I think this would be simpler as `if (!Line.startswith("#"))` or perhaps even `if (!Line.consume_front("#"))`


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:71-86
+  if (!Line.startswith("#line ") && !Line.startswith("# "))
+    return false; // Not a line directive (pragma etc).
+
+  // #line 123 "path/file.h"
+  // # 123 "path/file.h" 1
+
+  size_t Pos = 0;
----------------
I think all of this could be simplified to just a few lines.

Assuming you use the `consume_front` pattern above, then you could probably do the same here.  Something like:

```
Line.consume_front("line");
if (!Line.starts_with(" "))
  return false;
Line = Line.ltrim();  // There could be multiple spaces after the #line directive

size_t N;
if (Line.consumeInteger(10, N))  // returns true to signify an error
  return false;

Line = Line.ltrim();

// Then consume the filename and last integer.
```

It's messy manually dealing with strings and substrings, so using as much of `StringRef` builtin functionality as possible would make this code much more readable.


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:90-91
+  // Assuming we've found the quoted pathname.
+  if (Pos >= Line.size() || Line[Pos] != '"')
+    return false; // Malformed line, just skip it.
+
----------------
Do we not actually *use* the line number for anything?


Repository:
  rL LLVM

https://reviews.llvm.org/D46579