[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/test/tools/yaml2obj/preprocess.yaml:26
+# RUN: yaml2obj --docnum=2 %s | llvm-nm --just-symbol-name --no-sort - > %t.nosubst
+# RUN: echo -e 'a[[\n[[a\n[[a]\n[[a][[a]][[a]]' | cmp - %t.nosubst
+
----------------
MaskRay wrote:
> grimar wrote:
> > This does not work for me under windows.
> > 
> > echo -e 'a[[\n[[a\n[[a]\n[[a][[a]][[a]]' | cmp - %t.nosubst
> > 
> > I see the following when I run this line:
> > 
> > ```
> > D:\Work3\LLVM\llvm-project\build\test\tools\yaml2obj\Output>echo -e 'a[[\n[[a\n[
> > [a]\n[[a][[a]][[a]]' | cmp - preprocess.yaml.tmp.nosubst
> > - preprocess.yaml.tmp.nosubst различаются: байт 1, строка 1
> > ```
> > 
> > What means that byte 1 at the line 1 is different from the source. (My apologies, I do not know how to
> > switch to displaying errors in English for `cmp`).
> > 
> > I did not dig deeper because anyways I do not like this way of checking symbol names (sorry :),
> > so I'd ask you to rewrite in a more traditional way.
> Switched to 
> 
> ```
> echo -e '.........' > ...
> diff -u ... %t.nosubst
> ```
> 
> Unfortunately we cannot use FileCheck because FileCheck does not like  `[[` which is not used as a FileCheck pattern.
> Unfortunately we cannot use FileCheck because FileCheck does not like [[ which is not used as a FileCheck pattern.

Ah, I see. I did not know that. That is unfortunate behavior.


================
Comment at: llvm/tools/yaml2obj/yaml2obj.cpp:76
+
+  return Preprocessed;
+}
----------------
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73821