[PATCH] D156429: [TableGen] Add new bang operator !format
Francesco Petrogalli via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 27 08:13:52 PDT 2023
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
More information about the llvm-commits
mailing list