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

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:1180
+  auto BuiltinIt = BuiltinSymbolMap.find(IDLower);
+  auto VarIt = Variables.find(IDLower);
+  if (BuiltinIt != BuiltinSymbolMap.end()) {
----------------
Why do you do both lookups? Inserting into `Variables` with a built-in name errors, so we know that if it's in BuiltinSymbolMap, it can't be in Variables. So do

```
auto BuiltinIt = ...
if (BuiltinIt != end()) {
  ...
} else {
  auto VarIt = ...;
  if (VarIt != end() ... )
    ...
}
```


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:3654
+      VarIt = Variables.find(StringRef(Data).lower());
+      BuiltinIt = BuiltinSymbolMap.find(StringRef(Data).lower());
     }
----------------
Like above, looking up both in this loop looks weird to me. I'd structure this like

```
while (true) {
  // Lookup builtinText, check if not end, handle builtin case, continue
  // Same for variables
  break;
}


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7699
+    // Current local date, formatted MM/DD/YY
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
----------------
Please add a flag to set the the timestamp on the commandline, for deterministic build reasons. lld-link has `/timestamp:`, so I guess let's add that here too. (And then set TT to the arg of /timestamp: if it's passed and to time(nullptr) else)


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7700
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
+    SmallString<32> TmpBuffer;
----------------
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.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7703
+    llvm::raw_svector_ostream TmpStream(TmpBuffer);
+    TmpStream << llvm::format("%02d/%02d/%02d", TM->tm_mon + 1, TM->tm_mday,
+                              TM->tm_year % 100);
----------------
Instead of lines 7701-7705, do this:

```
char TmpBuffer[sizeof("mm/dd/yy")];
strftime(TmpBuffer, sizeof(TmpBuffer), "%D", TM);
return TmpBuffer;
```

Don't even need a dedicated variable for `TM` then.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7708
+  case BI_TIME: {
+    // Current local time, formatted HH:MM:SS
+    time_t TT = time(nullptr);
----------------
24-hour clock I guess? Not clear from the comment.


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7709
+    // Current local time, formatted HH:MM:SS
+    time_t TT = time(nullptr);
+    struct tm *TM = localtime(&TT);
----------------
`/timestamp:` support there too


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7711
+    struct tm *TM = localtime(&TT);
+    SmallString<32> TmpBuffer;
+    llvm::raw_svector_ostream TmpStream(TmpBuffer);
----------------
similar to above:

```
char TmpBuffer[sizeof("HH:MM:SS")];
strftime(TmpBuffer, sizeof(TmpBuffer), "%T", TM);
return TmpBuffer;
```


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7724
+  case BI_FILENAME:
+    return sys::path::stem(sys::path::filename(
+                               SrcMgr.getMemoryBuffer(SrcMgr.getMainFileID())
----------------
sys::path::stem() already extracts the filename, no need to call sys::path::filename() too. This is not supposed to include the extension, yes?


================
Comment at: llvm/lib/MC/MCParser/MasmParser.cpp:7727
+                                   ->getBufferIdentifier()))
+        .upper();
+  case BI_CURSEG:
----------------
Do we have to do the upper()?


================
Comment at: llvm/test/tools/llvm-ml/builtin_symbols.asm:43
+; CHECK: FileCur =
+; CHECK-SAME: builtin_symbols_t5.inc
+; CHECK: FileName =
----------------
Instead of the previous two lines, you could do `CHECK: FileCur = {{.*}}builtin_symbols_t5.inc` Could even use `{{.*[\\/]}}` to check that there's a directory separator in front of `builtin_symbols`


================
Comment at: llvm/test/tools/llvm-ml/builtin_symbols.asm:53
+; CHECK-LABEL: t6:
+; CHECK: Date = {{([[:digit:]]{2}/[[:digit:]]{2}/[[:digit:]]{2})}}
+; CHECK-NOT: {{[[:digit:]]}}
----------------
with `/timestamp:` you could put actual numbers here :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104965