- User Since
- Jul 27 2020, 8:22 AM (46 w, 3 d)
Thu, Jun 10
Wed, Jun 9
Tue, Jun 8
Mon, Jun 7
Thu, May 20
Wed, May 19
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."
Thanks, @dblaikie. I will incorporated your suggestions and the linties.
Apr 19 2021
Apr 17 2021
For my future reference: What is the sequence you go through to ensure that no target output files change? Do you just build with the current system, then build with the revision and check that no output file dates changed?
I presume this passes all the TableGen tests.
Apr 16 2021
I am pushing this revision today.
Apr 15 2021
Thanks! I will start a thread on llvm dev about how the union issue.
I like your improvements, I just want to do the first one last. It's easier for me to get all the assert-related code in and working, then go back and replace the tuple with a struct when all the relevant code is in one branch. I promise to do it and not just forget about it. It's next on my to-do list.
Apr 14 2021
In order to use a union in the RecordsEntry struct, I will have to add an enumerated value that specifies which kind of pointer is in the union. Is that worth the bother? Especially since I believe I'll have to write a special destructor.
Could I do the union as a prep patch, but please delay the change to a struct for a subsequent patch? This code is an extension to the previous steps that use a tuple. It's easier to think about the change to a struct as a separate patch.
Apr 13 2021
Could I get an LGTM on this revision?
Go for it!
Apr 12 2021
Oh, and LGTM.
I just pushed my bug fix. Let's wait a day until I know it doesn't bust the build, then push this one.