Add support for all built-in text macros supported by ML64:
@Date, @Time, @FileName, @FileCur, and @CurSeg.
Details
- Reviewers
thakis rnk - Commits
- rG5fba6058965c: [ms] [llvm-ml] Support built-in text macros
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/MC/MCParser/MasmParser.cpp | ||
---|---|---|
1184 | 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() ... ) ... } | |
3660 | 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; } | |
7712 | 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) | |
7713 | 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. | |
7716 | 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. | |
7721 | 24-hour clock I guess? Not clear from the comment. | |
7722 | /timestamp: support there too | |
7724 | similar to above: char TmpBuffer[sizeof("HH:MM:SS")]; strftime(TmpBuffer, sizeof(TmpBuffer), "%T", TM); return TmpBuffer; | |
7737 | sys::path::stem() already extracts the filename, no need to call sys::path::filename() too. This is not supposed to include the extension, yes? | |
7740 | Do we have to do the upper()? | |
llvm/test/tools/llvm-ml/builtin_symbols.asm | ||
44 | 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 | |
54 | with /timestamp: you could put actual numbers here :) |
llvm/lib/MC/MCParser/MasmParser.cpp | ||
---|---|---|
1184 | There seemed to be a lot of work going on in LLVM to avoid excessively-nested if statements, so I thought this was a worthwhile tradeoff. Thanks for correcting! Fixed. | |
7712 | I was adding that in a followup commit, but I've merged it in here now. | |
7713 | Switched to localtime_r; I don't think localtime has any real advantage here. | |
7721 | Clarified. | |
7737 | Interesting, I didn't realize stem did both. Fixed! | |
7740 | If we want to match ML.EXE, yes - it always uses an upper-cased form of the stem for the @Filename built-in. | |
llvm/test/tools/llvm-ml/builtin_symbols.asm | ||
44 | Good point; cleaned up. | |
54 | I had to add another flag for this, because MASM canonically uses the machine's local timezone for its built-ins; -utc now forces the use of GMT for the timezone. |
Switch back to gmtime/localtime (not thread-safe, but in a generally-safe location - and more cross-platform)
Also address strftime comments.
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