MaskRay marked an inline comment 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:
> > > 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.
> > std::gmtime(&TT); cannot return null
>
> I couldn't find good references for this. But on windows, it may return null
> (as the premerge CI shows). https://godbolt.org/z/jfxdG3KrM
Thanks:) This is interesting. So Windows gmtime returns null when the year
number is slightly larger than 3000 (I do not find the exact date...)
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
