Page MenuHomePhabricator

FileCheck [1/12]: Move variable table in new object
ClosedPublic

Authored by thopre on Apr 7 2019, 4:16 PM.

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch adds a new class to hold
pattern matching global state.

The table holding the values of FileCheck variable constitutes some sort
of global state for the matching phase, yet is passed as parameters of
all functions using it. This commit create a new FileCheckPatternContext
class pointed at from FileCheckPattern. While it increases the line
count, it separates local data from global state. Later commits build
on that to add numeric expression global state to that class.

Copyright:

  • Linaro (changes up to diff 183612 of revision D55940)
  • GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thopre added a comment.Apr 8 2019, 8:05 AM

Use camel casing for new functions

thopre updated this revision to Diff 194326.Apr 9 2019, 7:50 AM

Fix camel casing of modified functions as well

jhenderson added inline comments.Apr 10 2019, 5:37 AM
llvm/include/llvm/Support/FileCheck.h
100 ↗(On Diff #194326)

Is there going to be a getNumericVarValue or similar function in the future? If not, I think this should be simply getVarValue or getVariableValue.

102 ↗(On Diff #194326)

As numeric variables don't exist at this point, your comments should probably not refer to them. You can add the reference once they start becoming relevant here.

107 ↗(On Diff #194326)

Nit: ie -> i.e.

108 ↗(On Diff #194326)

Nit: you don't need the explicit void parameter.

111 ↗(On Diff #194326)

This forward declaration should probably be above your new class, so that your class sits next to the FileCheckPattern class.

138 ↗(On Diff #194326)

Just to be clear, this is shared between all patterns?

llvm/lib/Support/FileCheck.cpp
272 ↗(On Diff #194326)

I'd mention that the GlobalVariableTable is in the Context.

304 ↗(On Diff #194326)

I don't think you need to have Opt in the name, as it is fairly obviously an Optional from context. Maybe better would be VarValue.

307 ↗(On Diff #194326)

It is more typical to do if (!OptValue) for Optionals.

311 ↗(On Diff #194326)

More common is to do *OptValue.

389 ↗(On Diff #194326)

See comments above about the name and the usage immediately below.

477 ↗(On Diff #194326)

nothing -> None (to be explicit).

481 ↗(On Diff #194326)

"git" is a meaningless acronym to me. Perhaps simply "VarIter" or something like that would be better.

761 ↗(On Diff #194326)

unique_ptr or shared_ptr?

1387 ↗(On Diff #194326)

Is this necessary? Under what circumstances do we read the file twice?

1391 ↗(On Diff #194326)

Could you make this a StringRef here? That would simplify the code below.

1396 ↗(On Diff #194326)

Nit: ie -> i.e.

1399 ↗(On Diff #194326)

Don't use auto here. I don't know what the type is immediately.

thopre updated this revision to Diff 194507.Apr 10 2019, 6:55 AM
thopre marked 5 inline comments as done.
  • Make context a shared_ptr
  • Don't use auto for parameter of foreach loop if collection is not a local variable
  • Fix usage of Optional objects
  • Remove void function parameter
  • Remove references to pattern variable, be it in comment or identifiers
  • Fix ie -> i.e. typo
thopre marked 15 inline comments as done.Apr 10 2019, 6:57 AM

Thanks for such a quick review James! I addressed all your comments, except perhaps the one about guarding against DefineCmdlineVariables being called twice. Let me know if my fix seems sensible to you.

llvm/include/llvm/Support/FileCheck.h
100 ↗(On Diff #194326)

There is no getNumericVarValue in the future but this function is only for Pattern variable. Numeric variable use are represented by a pointer to the structure that holds the value so there is no need for a function. That said the term of pattern variable is introduced later so I could rename it later.

138 ↗(On Diff #194326)

Yes, updated the comment to make it clear.

llvm/lib/Support/FileCheck.cpp
1387 ↗(On Diff #194326)

I wanted to avoid this function being called twice by mistake and thus overriding variable values read from the input file by the value on the command-line. An assert is fine but it'll still need that static variable. Is that ok?

jhenderson added inline comments.Apr 10 2019, 7:07 AM
llvm/lib/Support/FileCheck.cpp
1416 ↗(On Diff #194507)

In this case you can just use the raw pointer referenced by the shared_ptr, as you don't need to own the pointer here.

1387 ↗(On Diff #194326)

I'd actually look at it a different way. Clear the variables list and only add the new values. That way this class is more flexible. The problem with a static local is that it is just global state with a local visibility. This would make it impossible to, for example, write unit tests that use this code. Speaking of which, how practical would adding unit tests be for the new code?

thopre updated this revision to Diff 194525.Apr 10 2019, 8:28 AM
thopre marked 2 inline comments as done.
  • Prevent variable definition from command-line switches if some variables are already defined
  • Use raw FileCheckPatternContext pointer to clear local variables
thopre marked an inline comment as done.Apr 10 2019, 8:30 AM
thopre marked an inline comment as done.Apr 11 2019, 2:23 AM

Have you looked at unit-testing the new code? It might be nice if you can.

llvm/lib/Support/FileCheck.cpp
1386–1387 ↗(On Diff #194525)

I'm okay with this, but an alternative method based on a different bit of code in the DWARFDebug* parser classes, might be to explicitly call GlobalVariableTable.clear() at the start of this function.

No need to change unless you want to.

1440 ↗(On Diff #194525)

Under what circumstances can Context be nullptr?

thopre marked 2 inline comments as done.Apr 11 2019, 4:51 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
1386–1387 ↗(On Diff #194525)

Command-line defines are called "GlobalDefines" in the FileCheckRequest API so my understanding is variables defined from processing the input file override the global defines rather than the reverse so no command-line definition should be processed once any variable has been defined from the input text.

Note that the API associate one FileCheckRequest to a given FileCheck class so 1 request = 1 input file and 1 directive file. There is thus no need to support the case of several input file sharing the same set of global defines. Since each input file gets its own FileCheckPatternContext, they will all get their own GlobalVariableTable which should be empty before the input text starts to be processed. This assert therefore does not get triggered in normal usage (global defined processed first) and will fire if global defines would be overriding variable defined from the input text.

I thus think this is a better approach.

1440 ↗(On Diff #194525)

When CheckStrings is empty but I've just realized thanks to you that readCheckFile guarantees it is not empty.

arsenm added a subscriber: arsenm.Apr 11 2019, 12:10 PM
arsenm added inline comments.
llvm/include/llvm/Support/FileCheck.h
138 ↗(On Diff #194326)

Why is this shared_ptr? For a context, there should be clear ownership. Each pattern doesn't need to have a potentially owning reference. It can live somewhere else

tra removed a subscriber: tra.Apr 11 2019, 1:16 PM
thopre updated this revision to Diff 194778.Apr 11 2019, 3:55 PM
thopre marked an inline comment as done.
  • Remove useless null check
  • Add unit tests
thopre marked an inline comment as done.Apr 11 2019, 3:59 PM
thopre marked an inline comment as done.Apr 11 2019, 4:14 PM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
138 ↗(On Diff #194326)

Hi Matt, thanks for the review!

The object is created in readCheckFile function and cease to be used in checkInput without a pointer being passed around explicitely. I used to delete the pointer taken from one of the pattern but James suggested to use a shared_ptr. It is overkill but allow the code to still work in case it is changed slightly. If the patterns hold a regular pointer I don't know in what shared_ptr to store the ownership and would need to revert to the initial approach.

Is there any other solution I've missed?

thopre marked an inline comment as done.Apr 11 2019, 4:16 PM
thopre added inline comments.
llvm/include/llvm/Support/FileCheck.h
138 ↗(On Diff #194326)

How about creating a pointer to the context in the FileCheck class? I'm tempted to make it a regular pointer since it is clear it can be destroyed once checkInput is finished but let me know if I should be using a shared_ptr instead.

arsenm added inline comments.Apr 12 2019, 12:59 AM
llvm/include/llvm/Support/FileCheck.h
138 ↗(On Diff #194326)

Yes, that's what I was thinking. Each instance needs access to the context, but it shouldn't potentially own it.

thopre updated this revision to Diff 194815.Apr 12 2019, 1:42 AM
thopre marked 2 inline comments as done.

Make Context a member of FileCheck class and stop using shared_ptr

Basically looks good to me. Just a few small comments remaining.

llvm/lib/Support/FileCheck.cpp
1381–1382 ↗(On Diff #194815)

I might be wrong, but I'm not sure you need (identical) doxygen comments in both source and header, so I think this and similar ones can be removed?

1393 ↗(On Diff #194815)

Nit: no need for the void parameter.

llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

I would add one last check to this test: re-add a local variable and show that you can fetch it again.

thopre updated this revision to Diff 194836.Apr 12 2019, 3:38 AM
thopre marked 4 inline comments as done.
  • Remove comments for new functions from cpp file
  • remove void from parameter list
thopre updated this revision to Diff 194847.Apr 12 2019, 5:22 AM

Fix comment to refer to None rather than nothing

probinson added inline comments.Apr 12 2019, 5:48 AM
llvm/lib/Support/FileCheck.cpp
1381–1382 ↗(On Diff #194815)

FTR, I'm quite sure doxygen comments go in the header if there is one. FileCheck was not tidied up when it moved to be a library, I think; would be worth a separate cleanup patch after the functional stuff is done, to get that all sorted.

jhenderson added inline comments.Apr 12 2019, 6:30 AM
llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

Okay, just waiting for this point to be addressed before I LGTM this.

thopre added inline comments.Apr 12 2019, 6:33 AM
llvm/lib/Support/FileCheck.cpp
1381–1382 ↗(On Diff #194815)

It is a maintenance nightmare too. However it is useful in the header to document the API and in the code as a comment. It seems to be also the case for existing functions. It does seem that in the rest of LLVM the comments are in the header usually, with occasional comment in the source as well. I have thus followed your suggestion and removed the comment in the cpp file, except when already present.

llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

The API does not provide a function to add a variable so I'm not sure how I would do that. FileCheckPattern is friend of FileCheckPatternContext so can add it directly so as to avoid creating a any user of the class to be able to add variables.

jhenderson added inline comments.Apr 12 2019, 6:40 AM
llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

Oh, right, yes of course. I forgot you decided to go the assert route in defineCmdlineVariables. Perhaps in another test you could run defineCmdlineVariables with just locals, clear them, then run it again?

thopre marked 3 inline comments as done.Apr 12 2019, 7:48 AM
thopre added inline comments.
llvm/lib/Support/FileCheck.cpp
1381–1382 ↗(On Diff #194815)

Which is what I did in this patch and am similarly removing comments from the cpp files in later patches.

llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

See my answer above. Do you want me to create a function to set a variable and make it public?

thopre updated this revision to Diff 194892.Apr 12 2019, 8:15 AM
thopre marked 2 inline comments as done.

Add redefinition unit testing

thopre marked 2 inline comments as done.Apr 12 2019, 8:15 AM
thopre added inline comments.
llvm/unittests/Support/FileCheckTest.cpp
44 ↗(On Diff #194815)

Oops, just saw your comment about local vars after I wrote the above message. Will fix that immediately

thopre marked an inline comment as done.Apr 12 2019, 8:16 AM
jhenderson accepted this revision.Apr 15 2019, 1:58 AM

LGTM, with one nit.

llvm/unittests/Support/FileCheckTest.cpp
47 ↗(On Diff #194892)

Either variables -> variable or remains -> remain (but not both).

This revision is now accepted and ready to land.Apr 15 2019, 1:58 AM
thopre updated this revision to Diff 195109.Apr 15 2019, 2:04 AM

Fix grammar in unit test comment

thopre marked an inline comment as done.Apr 15 2019, 2:04 AM
This revision was automatically updated to reflect the committed changes.