This is an archive of the discontinued LLVM Phabricator instance.

Store the "current position" index within the ASTRecordReader.
ClosedPublic

Authored by dlj on Dec 15 2016, 5:22 PM.

Details

Summary

For ASTDeclReader and ASTStmtReader, every parameter "unsigned &Idx" ultimately
comes from a variable that is defined on the stack, next to the RecordData. This
change moves that index into the ASTRecordReader.

TypeLocReader cannot be transitioned, due to TableGen-generated code which calls
ASTReader::GetTypeSourceInfo.

Event Timeline

dlj updated this revision to Diff 81694.Dec 15 2016, 5:22 PM
dlj retitled this revision from to Store the "current position" index within the ASTRecordReader..
dlj updated this object.
dlj added a reviewer: rsmith.
dlj added a subscriber: cfe-commits.
rsmith edited edge metadata.Dec 16 2016, 5:14 PM

I like moving Idx into the reader, but I have mixed feelings about the iterator-like interface. For consistency with the rest of the ASTRecordReader interface, and with how we model the writer side, I think perhaps Record.ReadInt() would fit better than using *Record++.

dlj updated this revision to Diff 81870.Dec 17 2016, 8:44 PM
dlj edited edge metadata.
  • Removed iterator syntax.
dlj added a comment.Dec 17 2016, 8:53 PM

Yeah, that makes more sense. Switched to readInt/peekInt/skipInts, let me know if you have a better idea for the names.

dlj updated this revision to Diff 82049.Dec 19 2016, 5:34 PM
  • Switched (void)Reader.readInt() to Reader.skipInts(1)
rsmith accepted this revision.Dec 20 2016, 11:30 AM
rsmith edited edge metadata.

LGTM, any chance I can tempt you to lowerCamelCase all the other ASTRecordReader members to match the new ones as a follow-up change?

This revision is now accepted and ready to land.Dec 20 2016, 11:30 AM
dlj added a comment.Dec 20 2016, 4:18 PM

LGTM, any chance I can tempt you to lowerCamelCase all the other ASTRecordReader members to match the new ones as a follow-up change?

Yup, will do.

This revision was automatically updated to reflect the committed changes.