[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
Thu Feb 6 01:05:41 PST 2020
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
More information about the llvm-commits
mailing list