This is an archive of the discontinued LLVM Phabricator instance.

Update TableGen LangIntro.rst
ClosedPublic

Authored by chenwj on Apr 16 2017, 1:40 AM.

Details

Summary

Update a few places in TableGen LangIntro.rst.

  • Only one-line double-quoted string literal considered as string type, see how check is done in TGLexer::LexString() (TGLexer.cpp).
  • string type value can be converted to code type, but not the other way around. See StringRecTy::classof and CodeRecTy::classof (Record.h).

Diff Detail

Repository
rL LLVM

Event Timeline

chenwj created this revision.Apr 16 2017, 1:40 AM
chenwj edited the summary of this revision. (Show Details)Apr 16 2017, 1:42 AM
chenwj added reviewers: stoklund, t.p.northover.
chenwj edited the summary of this revision. (Show Details)
chenwj edited the summary of this revision. (Show Details)
chenwj updated this revision to Diff 95400.Apr 16 2017, 1:46 AM

fix typo.

Ping? Any comment? :-)

kparzysz added inline comments.
docs/TableGen/LangIntro.rst
132 ↗(On Diff #95400)

How about "access to an ordered sequence of bits of a value, in particular value{15-17} produces an order that is the reverse of value{17-15}."

asb added a reviewer: asb.EditedApr 25 2017, 4:55 AM
asb added a subscriber: asb.

Hi, thanks for the patch. My feedback is:

  1. Limitations of literal formats shouldn't be discussed when introducing the types.
  2. Perhaps outside the scope of this patch, but it does seem a little odd that a simple string literal can be assigned to either a string or code variable, but code fragment literals can't be assigned to string variables (at least on the couple of months old build on LLVM I have on this machine). The existing description that a code fragment is "just a multiline string literal" seems incorrect due to this.
docs/TableGen/LangIntro.rst
58 ↗(On Diff #95400)

It's correct that string literals can't span multiple lines, but I think this really belongs in the discussion later in the document about TableGen values and expressions which documents literal formats.

Also, does the document mention expressions like "Inst{31,27-25,14}", where the bit accesses are in a comma-separated list?

chenwj updated this revision to Diff 96910.Apr 27 2017, 6:24 AM

Address review comments.

Also, does the document mention expressions like "Inst{31,27-25,14}", where the bit accesses are in a comma-separated list?

As far I can tell, no. I don't see such example mentioned in this document. Would you like to add one?

In D32117#736622, @asb wrote:

Hi, thanks for the patch. My feedback is:

  1. Limitations of literal formats shouldn't be discussed when introducing the types.
  2. Perhaps outside the scope of this patch, but it does seem a little odd that a simple string literal can be assigned to either a string or code variable, but code fragment literals can't be assigned to string variables (at least on the couple of months old build on LLVM I have on this machine). The existing description that a code fragment is "just a multiline string literal" seems incorrect due to this.

I think I already addressed the item one? As for item two, how do you want to rephrase it?

As far I can tell, no. I don't see such example mentioned in this document. Would you like to add one?

Probably not in this review. Otherwise you could keep updating it forever. From my point of view, LGTM.

@asb And you? :-)

asb accepted this revision.Apr 28 2017, 12:13 AM

Thank you @chenwj, this is definitely an improvement over the current documentation. Would you like me to commit?

This revision is now accepted and ready to land.Apr 28 2017, 12:13 AM
chenwj added a comment.EditedApr 28 2017, 5:09 AM
In D32117#740495, @asb wrote:

Thank you @chenwj, this is definitely an improvement over the current documentation. Would you like me to commit?

@asb Go ahead! Thanks. :-)

This revision was automatically updated to reflect the committed changes.
asb added a comment.May 2 2017, 7:01 AM

Patch committed, many thanks for your contribution @chenwj.

chenwj added a comment.May 2 2017, 7:07 AM
In D32117#743341, @asb wrote:

Patch committed, many thanks for your contribution @chenwj.

Thanks. ;)