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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:76
+
+  return Preprocessed;
+}
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > I'd suggest something like the following
> > > 
> > > ```
> > > std::vector<std::pair<std::string, StringRef>> Substitution;
> > >   for (StringRef Define : D) {
> > >     StringRef F, V;
> > >     std::tie(F, V) = Define.split('=');
> > >     if (!Define.count('=') || F.empty()) {
> > >       ErrHandler("bad override, missing field name: " + Define);
> > >       return {};
> > >     }
> > >     Substitution.push_back({(Twine("[[") + F + "]]").str(), V});
> > >   }
> > > 
> > >   std::string Document = Buf.str();
> > >   if (Substitution.empty())
> > >     return Document;
> > > 
> > >   for (std::pair<std::string, StringRef> &P : Substitution) {
> > >     while (true) {
> > >       auto I = Document.find(P.first);
> > >       if (I == std::string::npos)
> > >         break;
> > >       Document = Document.replace(I, P.first.size(), P.second.str());
> > >     }
> > >   }
> > > ```
> > Maybe we should avoid quadratic complexity behavior, just in case that there are a long run of `[[[[[[[[[[`.
> > just in case that there are a long run of [[[[[[[[[[.
> 
> It reminded me about D36307. I believe that `[[[[[[` is a misuse of the feature and hence we probably should not care much.
> The approach I suggested is just very simple and it is much easier to read. I think the common use case for this feature will be
> "replace 1-2 defines in a short enough YAML", i.e. I am not sure we should think about an additional optimizations here.
> 
> Probably it is time to hear a third opinion though. @jhenderson, what do you think?
> 
Doing `replace` repeatedly may also have a non-convergence problem...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73821