- User Since
- Jul 27 2020, 8:22 AM (101 w, 1 d)
May 24 2022
I think my approval is sufficient.
Looks good to me.
May 23 2022
May 17 2022
I think you can use defvar to define two constants strue and sfalse, then use them instead of true and false. However, defvar'ed constants are a bit funky, so you'll have to test it.
May 16 2022
The 'true' and 'false' literals are available.
May 9 2022
You are good to go.
Apr 23 2022
Sorry I've been scarce for this patch. Doesn't it need documentation?
Apr 12 2022
Sorry, I'm pretty busy now and don't have time right away to work on this. Would it make sense to document it now? Then we can determine whether the two schemes can be merged in a reasonable way.
Apr 8 2022
Why don't you take an inventory of the number of uses of GenericEnum? Then we can see if it's practical to replace those uses.
Apr 7 2022
So one possibility is to deprecate GenericEnum, replace all uses of it with your Enum, and document Enum as the standard solution.
Apr 1 2022
Sorry, I'm just quickly passing through for now. How does this new feature relate to the existing GenericEnum class?
Feb 8 2022
Looks good to me.
Sorry, I don't know what the CI test suite is. I don't know much about the testing facilities.
Did you run all the TableGen tests?
Jan 7 2022
ProgRef.rst looks good to me.
Thanks for making these corrections to the TableGen document. See my comments above.
Nov 24 2021
The change to ProgRef.rst looks good to me. The other two changes look fine, though I do not "own" those files.
Nov 18 2021
Nov 4 2021
Doesn't C++ allow unnamed parameters? That would be another way to do it. And that would make it impossible to reference the template argument name.
I mean something really explicit, like a statement especially designed for this.
I vote for something explicit. An unused field seems like a hack, no?
I suggested an 'unused' statement above. With it, we should be able to make this option on by default.
Sep 8 2021
@c-rhodes Oh, absolutely, no need for a new statement until we determine whether there are legitimate reasons for unused template arguments. In fact, if we determine that there are none, we could make the new option on by default.
Sep 7 2021
I would propose something radical like a new statement:
(I've been scarce due to summer holidays and, sadly, real work.)
Jun 10 2021
Jun 9 2021
Jun 8 2021
Jun 7 2021
May 20 2021
May 19 2021
Sorry, I removed it but didn't update this review. All set now.
May 18 2021
Could I get an LGTM on this? I have removed the spurious comment.
May 13 2021
May 12 2021
One must amend the commit when one makes changes to the files.
The arm_*.inc files do not change with this revision.
May 11 2021
I couldn't find anything in the actions to "restart review." Did I miss it?
I'll start a new Phabrication for the change.
I reverted this commit. There is at least one build that uses a 'sed' that interprets \x00 as "x00" and so does not substitute a NUL character.
May 10 2021
I used David's trick to eliminate the NUL characters in the test file. They are now represented by at signs (@) and translated on the fly.
May 7 2021
Hang on, there is already a TableGen test that uses sed: intrin-properties.td. So obviously I have sed in the right place.
No, I was confused, thinking that the modified file would go back through Lit. But of course it doesn't. I will set up something like you suggest. I'm sure there is a way to use an escape sequence to represent the NUL character in sed . . . yes, \000.
There are test files that use sed to do replacements. I'll ask on llvm dev how to create a RUN: line that does the right thing. It has to replace @ with NUL and get rid of the sed command so it doesn't recurse forever. My Unix command knowledge is meager.
Okay, sounds good. I'll see what I can find.
There are four or five NULs in the test file, to check them in various positions. Would it be helpful to add a second nul-char.txt file that is a duplicate, except that it has an at sign (@) where every null is? I could refer to it with a comment in nul-char.td.
The TableGen frontend has no way of knowing whether a particular backend, will use a record, nor any way of knowing whether no backends at all will use it. I'm not sure how we could detect unused records.
May 6 2021
LGTM. Just please verify that it passes all the TableGen tests.
Nice cleanup! I presume this passes all the TableGen tests?
I added a test. There are NULs in the .td file.
May 5 2021
Restored whitespace as requested by Jan.
Now the //// comments should be gone.
Hmm. I swore I deleted all the //// lines. Hang on . . .
May 4 2021
May 3 2021
Apr 30 2021
Apr 28 2021
Apr 27 2021
I will push this on Wednesday.
@paperchalice: I don't understand the purpose of this revision. Could you explain in more detail?
Apr 26 2021
It makes good sense to spend some time now on additional TableGen file cleanup. There is one file with TODOs for assert, so I will start there.
I added !find() to complement !substr(), so that TableGen would have the two most common string functions. I believe !find will be helpful with 'assert' as people begin adding assertions to TableGen files.
Apr 23 2021
Apr 22 2021
Ah, so a trivial change like this doesn't need review. Thanks!
I have incorporated the lint changes and David's suggestions.
I added a note about the term "parent class." It proved futile to try to remove the word "parent" in certain sentences. Many pertain to both classes and records. And those that don't just seemed to become inconsistent when I tried to remove the word.
Apr 21 2021
I'm going to stick with "parent class," but if there are any sentences that talk only about records, I will try to use just "class." I will add a note at the appropriate spot explaining this.
Apr 20 2021
The more I think about it, the more I like "parent class" rather than "base class." It's the only term that seems like it could apply to a record's class. Let me look at the uses and see if some or most can just use the term "class."