This is an archive of the discontinued LLVM Phabricator instance.

[Fixed Point] [AST] Add an AST serialization code for fixed-point literals.
ClosedPublic

Authored by ebevhan on Jan 25 2019, 3:39 AM.

Details

Summary

This patch adds the EXPR_FIXEDPOINT_LITERAL AST
code to serialize FixedPointLiterals. They were previously
being serialized with the code for integer literals, which
doesn't work properly.

Diff Detail

Event Timeline

ebevhan created this revision.Jan 25 2019, 3:39 AM
ebevhan marked an inline comment as done.Jan 25 2019, 3:41 AM
ebevhan added inline comments.
include/clang/Serialization/ASTBitCodes.h
1637 ↗(On Diff #183511)

I'm unsure if this is the right location for a new code. This will bump down all the other codes below this and cause any older file to not be read correctly.

Should the file format version number be bumped up?

rjmccall added inline comments.Jan 25 2019, 9:27 AM
include/clang/Serialization/ASTBitCodes.h
1637 ↗(On Diff #183511)

IIRC the real file format version for this is based on the exact revision of the compiler and there is no pretense of serialized ASTs being sharable between compilers. I don't think we've ever bumped the version number for incompatible changes like this.

leonardchan accepted this revision.Feb 21 2019, 3:19 PM

LGTM. Everything still seems to work on my end after applying this diff.

This revision is now accepted and ready to land.Feb 21 2019, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 3:19 PM
bjope added a subscriber: bjope.Oct 9 2019, 7:55 AM

https://reviews.llvm.org/D77721 depends on this serialization change for fixed point literals.

https://reviews.llvm.org/D77721 depends on this serialization change for fixed point literals.

You could probably take over and land the patch if @ebevhan doesn't respond back soon when you get LGTM for D77721. If you do, just be sure to credit him :)

Some minor requests, but otherwise this LGTM.

include/clang/Serialization/ASTBitCodes.h
1637 ↗(On Diff #183511)

But really you should add the enumerator to the end of the list.

test/PCH/fixed-point-literal.h
1 ↗(On Diff #183511)

Inputs like this generally go into the Inputs directory.

Address comments from rjmccall

rjmccall added inline comments.Apr 13 2020, 8:31 PM
clang/include/clang/Serialization/ASTBitCodes.h
1892

Thanks!

I'm sorry to be extremely picky about this, but:

  • Please put the comment on the same line and just make it // FixedPointLiteral, like all of the other node-specific comments.
  • I think a reformatting of the other comments would be welcome, but if you're going to do it, please do it in a separate commit, and please just indent all of the comments to a consistent column.

update based on comments from rjmccall

Thank you for the comments. Please do let me know when this patch is ready to land. Best!

rjmccall accepted this revision.Apr 14 2020, 10:30 AM

LGTM, thanks.

This revision was automatically updated to reflect the committed changes.