MaskRay marked 2 inline comments as done. MaskRay added inline comments.
================ Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1092 + TT = *PP.getPreprocessorOpts().SourceDateEpoch; + TM = std::gmtime(&TT); + } else { ---------------- ychen wrote: > MaskRay wrote: > > ychen wrote: > > > Why not use localtime as the else branch? > > > > > > As mentioned above, diagnose null TM here or when parsing the > > > user-specifed value. A small trade-off to make, up to you. > > Can you elaborate? With the check in Frontend I think a check here is not > > necessary... > Sorry I was not clear. The time_t value is not portable so it is hard to > diagnose in a target-independent way. For example, `253402300799` is too big > of a value for windows that `gmtime` returns null (causes crash at `asctime` > in the similar case below). We could diagnose when gmtime/localtime returns > null, which is portable. I am still confused. In practice, a 64-bit `gmtime` implementation may returns null when `tm_year` would overflow while 32-bit `gmtime` can't. The way (with a more restricted upper bound) we check SOURCE_DATE_EPOCH in Frontend, `std::gmtime(&TT);` cannot return null. `localtime(&TT)` for `time(nullptr)` may overflow as where but that is an pre-existing issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135045/new/ https://reviews.llvm.org/D135045 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits