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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


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


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


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:54
+    if (parseLine(Line))
+      Result += FullLine;
+  }
----------------
rnk wrote:
> This is O(n^2). This shouldn't need any copies until it's time to join the string, so you can make a `std::vector<StringRef>` and then use `return llvm::join(Lines, "")`, right?
That works just as well, thanks!


================
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;
----------------
zturner wrote:
> 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.
Thanks! Yes, I tried to use as much of the StringRef builtins as possible, but didn't really find the right ones that actually makes it this simple.

With more of ltrim, consume_front/consumeInteger and rsplit, I managed to make it really short and succinct.


================
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.
+
----------------
zturner wrote:
> Do we not actually *use* the line number for anything?
Nope, I don't use it anywhere (yet).

If we'd want to have proper line numbers in error reports we'd need to keep these lines and handle them in the tokenizer somehow I guess, but I'm just aiming at the minimal functionality for this for now.


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.cpp:106
+  StringRef Ext = Path.substr(ExtPos + 1);
+  if (Ext.equals_lower("h")) {
+    Outputting = false;
----------------
amccarth wrote:
> also "c"
Done


================
Comment at: tools/llvm-rc/ResourceScriptCppFilter.h:16
+// headers can leave behind C declarations, that RC doesn't understand.
+// Rc.exe simply discards anything that comes from files named *.h.
+//
----------------
amccarth wrote:
> Also from files name *.c.  See last paragraph on:
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa381033(v=vs.85).aspx
> 
> This is a hack from the old days.  Instead of putting your UI IDs into a header, you'd just define them in the .c file that implements that part of the UI, knowing that the .c file could be included in the resource script.
Whoa, color me surprised, I hadn't expected this to be documented.


Repository:
  rL LLVM

https://reviews.llvm.org/D46579