[PATCH] D104965: [ms] [llvm-ml] Support built-in text macros

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.
This revision is now accepted and ready to land.

Looks good now except for below.



================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7719
+    char TmpBuffer[sizeof("hh:mm:ss")];
+    const size_t Len = strftime(TmpBuffer, sizeof(TmpBuffer), "%H:%M:%S", &TM);
+    return std::string(TmpBuffer, Len);
----------------
Does `"%T"` not work as format string?


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7700
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
+    SmallString<32> TmpBuffer;
----------------
epastor wrote:
> thakis wrote:
> > Hm, this makes this function non-thread-safe. I think that's fine, but maybe add a `// Not thread-safe` at the end of the line to make that explicit.
> Switched to `localtime_r`; I don't think `localtime` has any real advantage here.
Is that available on all platforms? If so, ok.

Does `"%D"` for format string not work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104965