Page MenuHomePhabricator

[lldb] s/FileSpec::Equal/FileSpec::Match
ClosedPublic

Authored by labath on Nov 29 2019, 5:12 AM.

Details

Summary

The FileSpec class is often used as a sort of a pattern -- one specifies
a bare file name to search, and we check if in matches the full file
name of an existing module (for example).

These comparisons used FileSpec::Equal, which had some support for it
(via the full=false argument), but it was not a good fit for this job.

For one, it did a symmetric comparison, which makes sense for a function
called "equal", but not for typical searches (when searching for
"/foo/bar.so", we don't want to find a module whose name is just
"bar.so"). This resulted in patterns like:

if (FileSpec::Equal(pattern, file, pattern.GetDirectory()))

which would request a "full" match only if the pattern really contained
a directory. This worked, but the intended behavior was very unobvious.

On top of that, a lot of the code wanted to handle the case of an
"empty" pattern, and treat it as matching everything. This resulted in
conditions like:

if (pattern && !FileSpec::Equal(pattern, file, pattern.GetDirectory())

which are nearly impossible to decipher.

This patch introduces a FileSpec::Match function, which does exactly
what most of FileSpec::Equal callers want, an asymmetric match between a
"pattern" FileSpec and a an actual FileSpec. Empty paterns match
everything, filename-only patterns match only the filename component.

I've tried to update all callers of FileSpec::Equal to use a simpler
interface. Those that hardcoded full=true have been changed to use
operator==. Those passing full=pattern.GetDirectory() have been changed
to use FileSpec::Match.

There was also a handful of places which hardcoded full=false. I've
changed these to use FileSpec::Match too. This is a slight change in
semantics, but it does not look like that was ever intended, and it was
more likely a result of a misunderstanding of the "proper" way to use
FileSpec::Equal.

[In an ideal world a "FileSpec" and a "FileSpec pattern" would be two
different types, but given how widespread FileSpec is, it is unlikely
we'll get there in one go. This at least provides a good starting point
by centralizing all matching behavior.]

Event Timeline

labath created this revision.Nov 29 2019, 5:12 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
jdoerfert resigned from this revision.Nov 29 2019, 10:10 AM
JDevlieghere accepted this revision.Dec 2 2019, 10:12 AM

LGTM assuming the answer to my inline comment is negative :-)

lldb/include/lldb/Utility/FileSpec.h
196

Why do we still need the Equal method? Are there cases where full is only decided at runtime? Would it be worth to update the call sites to use == or ::Match directly? I think having both Match and Equal with these semantics is confusing and will likely reintroduce the things you just cleaned up.

This revision is now accepted and ready to land.Dec 2 2019, 10:12 AM
clayborg added inline comments.
lldb/include/lldb/Utility/FileSpec.h
196

I agree with JDev here.

202

Maybe rename to "Matches"?

labath marked 5 inline comments as done.Dec 3 2019, 7:59 AM
labath added inline comments.
lldb/include/lldb/Utility/FileSpec.h
196

I also wanted to delete it completely, but then I ran into FileSpecList::FindFirstFile, which forwards the full parameter to this function. I believe the calls to *that* function all have "static" values of the full argument, but the way this argument is used in this function is so convoluted, I thought I'd be best to leave that for a separate patch. (I'm pretty sure the convolutedness is not intentional, but also a result of the misunderstanding of how FileSpec::Equal works, but that means that fixing *that* will not be NFC.

202

Match seems to be the prevalent choice in other match-like apis (re.match, pcre_match, etc.).

Okay, sound sensible. LGTM!

This revision was automatically updated to reflect the committed changes.
labath marked 2 inline comments as done.