[PATCH] D156429: [TableGen] Add new bang operator !format

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

Thank you for implementing this. I have added just a couple of comments.

Francesco



================
Comment at: llvm/docs/TableGen/ProgRef.rst:1731
 
+``!format(``\ *fmt*\ ``,`` *args*\ ``, ...)``
+    Format ``args`` according to the format string ``fmt``, and return the
----------------
This is really nitpicking. You allow (and I think it is correct) invocations without *args*:

```
 string formatNoArg       = !format("");
```

... so maybe replace this with the suggestion? This is how it is done for printf-like variadic functions: https://en.cppreference.com/w/c/io/fprintf


================
Comment at: llvm/include/llvm/TableGen/Record.h:1275-1276
+
+  size_t args_size() const { return NumArgs; }
+  bool args_empty() const { return NumArgs == 0; }
+
----------------
Do we need this API? It doesn't seem to be used anywhere... (am I missing something?)


================
Comment at: llvm/lib/TableGen/Record.cpp:1618
+  std::string Res = std::string(Target);
+  std::string::size_type Found;
+  std::string::size_type Index = 0;
----------------
`Found` sounds like boolean to me. How about `Pos`?


================
Comment at: llvm/lib/TableGen/Record.cpp:2090
+    for (unsigned I = 0; I < NumArgs; I++) {
+      auto *Arg = getArg(I);
+      // FIXME: This is a really simple string substitution. Shoule we make it
----------------
Avoid `auto`, cannot get the type information from the RHS. 


================
Comment at: llvm/lib/TableGen/Record.cpp:2135
+  Result += Fmt->getAsString();
+  for (auto *Arg : args())
+    Result += ", " + Arg->getAsString();
----------------
Same, no type info from the invocation of `args()`. Avoid the use of `auto`


================
Comment at: llvm/test/TableGen/format.td:28
+  string formatOperatorFmt = !format(!if(!not(bitValue), "{0} {1}", "{1} {0}"), 0, 1);
+  string formatPasteFmt    = !format("{" # "0" # "}", 0);
+  code   formatCodeFmt     = !format([{
----------------
Nit: can we test these with values that do not correspond to the actual placeholder number? maybe replacements like the one suggested.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156429