This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Use simple string matching instead of Regex
ClosedPublic

Authored by kbobyrev on Aug 28 2018, 8:17 AM.

Details

Summary

Instead of parsing and compiling the llvm::Regex each time, it's faster to use basic string matching for filename prefix check.

Diff Detail

Event Timeline

kbobyrev created this revision.Aug 28 2018, 8:17 AM

Thanks very much for improving it!

clang-tools-extra/clang-tidy/abseil/AbseilMatcher.h
45

The logic is a bit subtle here. Agree that using regex is overkill.

We still want substring match here, since the Filename does not always start with absl/, so your improvement will change the behavior. I think we should do absl/[absl-libraries] substring match.

kbobyrev updated this revision to Diff 163302.Aug 30 2018, 4:38 AM
kbobyrev marked an inline comment as done.

Ah, correct. I totally forgot about the ^string$ for the exact match.

This should not change behavior now. I believe what you propose would make the code easier (there'd only be AbseilLibraries = {"absl/algorithm", ...} and the final return std::any_of(..., [](...) { return Path.find(Library) != StringRef::npos; }); but the performance would be worse (because of the presence of the absl/ prefix in each entry). Would you consider this less efficient but easier-to-follow approach better?

hokein accepted this revision.Aug 30 2018, 5:18 AM

Ah, correct. I totally forgot about the ^string$ for the exact match.

This should not change behavior now. I believe what you propose would make the code easier (there'd only be AbseilLibraries = {"absl/algorithm", ...} and the final return std::any_of(..., [](...) { return Path.find(Library) != StringRef::npos; }); but the performance would be worse (because of the presence of the absl/ prefix in each entry). Would you consider this less efficient but easier-to-follow approach better?

It was my suggestion. I'm fine with the current way as long as we have a clear comment explaining it.

clang-tools-extra/clang-tidy/abseil/AbseilMatcher.h
44–53

I think we should have a comment explaining we are matching absl/[absl-libraries] -- the current code is not as straightforward as before.

This revision is now accepted and ready to land.Aug 30 2018, 5:18 AM
kbobyrev updated this revision to Diff 163309.Aug 30 2018, 5:35 AM
kbobyrev marked an inline comment as done.

Mention that the code actually performs absl/(any|absl|library) substring match.

This revision was automatically updated to reflect the committed changes.