[PATCH] D73821: [yaml2obj] Add -D k=v to preprocess the input YAML

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/yaml2obj/preprocess.yaml:1
+# RUN: yaml2obj -D NAME= %s | llvm-nm - | FileCheck --check-prefix=FBAR %s
+# FBAR: U fbar
----------------
Perhaps also worth a test case that "-DNAME=..." is accepted, since that matches the compiler syntax, so would be a likely usage.


================
Comment at: llvm/test/tools/yaml2obj/preprocess.yaml:22
+
+## FileCheck uses [[ as a pattern. It cannot be used for [[ testing.
+## Test that undefined patterns are not substituted.
----------------
I'm not sure this comment line adds anything. My immediate reaction was "what's this test got to do with FileCheck?"


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:35
+static cl::list<std::string>
+    D("D", cl::desc("list of overridden fields: field=value"));
+
----------------
grimar wrote:
> Seems the description should start from the upper case letter and perhaps I'd reword it somehow.
> It is not a list it seems? It is a single redefinition as far I understand.
I'd change this help text to "Defined the specified macros to their specified definition. The syntax is <macro>=<definition>".


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:48
+  for (StringRef Define : D) {
+    StringRef F, V;
+    std::tie(F, V) = Define.split('=');
----------------
grimar wrote:
> What is `F` stands for? I`d rename to `Name` and `Value`..
Let's use `Macro` and `Definition`

The "field/value" naming doesn't really make sense since the new functionality is much more general purpose.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:51
+    if (!Define.count('=') || F.empty()) {
+      ErrHandler("bad override, missing field name: " + Define);
+      return {};
----------------
This error should a) mention the switch name, and b) probably not use the word "override" since nothing is being overridden as such. How about "invalid syntax for -D: xxx" (where xxx is the input string). If you think you need extra context, I'd split "missing '=' from "missing macro name".


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:61
+      size_t I = Buf.find("]]", 2);
+      if (I != size_t(-1)) {
+        StringRef Macro = Buf.substr(2, I - 2);
----------------
`I != StringRef::npos`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73821/new/

https://reviews.llvm.org/D73821