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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:54
+                                        yaml::ErrorHandler ErrHandler) {
+  DenseMap<StringRef, StringRef> Defines;
+  for (StringRef Define : D) {
----------------
grimar wrote:
> Should we exit early for a general case?
> 
> ```
>   if (D.empty())
>     return Buf.str();
> ```
> 
> It might simplify things for those who want to debug the code.
When an early exit is just for the fast path (for the common case), I prefer not to add them.

For example,

```
if (thing.empty())
  return;
for (... : thing) {
  do something
}
```

I think `if (thing.empty())` is unnecessary, unless the code is a critical path and the early return thing has significant performance improvement.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:74
+      auto It = Defines.find(Macro);
+      if (It != Defines.end() && Buf.substr(I).startswith("]]")) {
+        Preprocessed += It->second;
----------------
grimar wrote:
> Here is a little logical issue. If we have input like "[[AAA[",
> then you still assign "AAA" to `Macro` and perform a lookup,
> but at fact "AAA" is not a macro and you do not need to do a lookup for it.
Yes. The `if` can be split to avoid a lookup. It will add two lines. I can do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73821