[PATCH] D73821: [yaml2obj] Add -D k=v to preprocess the input YAML
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 4 01:17:38 PST 2020
grimar added a comment.
I am going to debug this. My first impression is that the direction is right,
but it needs a bit of polishing.
Will add more comments soon.
================
Comment at: llvm/test/tools/yaml2obj/ELF/emachine.yaml:1
## Test how the Machine YAML field can be used to set the e_machine ELF header field.
----------------
This is a refactoring change, right? I'd leave such changes for a follow-up patch(es).
================
Comment at: llvm/test/tools/yaml2obj/preprocess.yaml:10
+
+# RUN: not yaml2obj -D =value %s 2>&1 | FileCheck --check-prefix=ERR2 %s
+# ERR2: error: bad override, missing field name: =value
----------------
It is probably usefull to add cases with more than one `-D`:
```
-D FOO=x -D BAR=y
```
```
-D FOO=x -D FOO=y
```
================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:35
+static cl::list<std::string>
+ D("D", cl::desc("list of overridden fields: field=value"));
+
----------------
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.
================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:46
+ yaml::ErrorHandler ErrHandler) {
+ DenseMap<StringRef, StringRef> Substitution;
+ for (StringRef Define : D) {
----------------
Perhaps `Substitutions` or may be just `Defines`? Up to you,
but `-D` option is closer to the latter word I think.
================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:48
+ for (StringRef Define : D) {
+ StringRef F, V;
+ std::tie(F, V) = Define.split('=');
----------------
What is `F` stands for? I`d rename to `Name` and `Value`..
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73821/new/
https://reviews.llvm.org/D73821
More information about the llvm-commits
mailing list