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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:54
+                                        yaml::ErrorHandler ErrHandler) {
+  DenseMap<StringRef, StringRef> Defines;
+  for (StringRef Define : D) {
----------------
MaskRay wrote:
> 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.
I see what you mean, though here the situation is a bit different probably: too many code is involved, i.e. it is not a trivial loop from your example. it is somehting much more. I do not insist here though, so up to you.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:74
+      auto It = Defines.find(Macro);
+      if (It != Defines.end() && Buf.substr(I).startswith("]]")) {
+        Preprocessed += It->second;
----------------
MaskRay wrote:
> 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.
I am not concerned about the perfomance at all here, but I'd do that for code readability as it is a bit too noticable and IMO might look like a possible issue for a new readers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73821