This is an archive of the discontinued LLVM Phabricator instance.

Enhance abseil-faster-strsplit-delimiter to handle other non-printable characters.
ClosedPublic

Authored by zhangxy988 on Jul 3 2019, 11:39 AM.

Details

Summary

Currently it fails on cases like '\001'.

Note: Since StringLiteral::outputString dumps most nonprintable
characters in octal value, the exact string literal format isn't preserved,
e.g. "\x01" becomes '\001'.

Event Timeline

zhangxy988 created this revision.Jul 3 2019, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 11:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gribozavr accepted this revision.Jul 3 2019, 11:45 AM

Do you have commit access? Would you like me to commit this patch for you?

This revision is now accepted and ready to land.Jul 3 2019, 11:45 AM

I don't think I have commit access.
Please commit it for me. Thanks!

Sorry, the patch does not apply cleanly to current master -- could you rebase please?

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
10

Unnecessary empty line.

lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
26–27

Assertion messages missing

clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
55

Is there a negative test, absl::StrSplit("ABC", R"(AA)");?
Also what about wide chars? (the second assertion)

zhangxy988 updated this revision to Diff 208500.Jul 8 2019, 1:56 PM
zhangxy988 marked 2 inline comments as done.
  • Remove empty line and add assertion message.
zhangxy988 marked an inline comment as done.Jul 8 2019, 1:58 PM
zhangxy988 added inline comments.
clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
55

I don't really know how to write a negative test like that.
This check is only concerned with single character string literal and it doesn't support wide char (since StrSplit doesn't).

lebedev.ri added inline comments.Jul 9 2019, 12:24 AM
clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
55

What i'm asking is, those assertions are there just to say "look, i guarantee those invariants hold here",
or to say "oh no, we shouldn't have ever violated these invariants here"? The difference being, in former case,
there is some other error checking that prevents those assertions from triggering; in latter case there is not.

gribozavr marked an inline comment as done.Jul 9 2019, 3:51 AM
gribozavr added inline comments.
clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
55

There's a test below for multi-character string literals, absl::StrSplit("ABC", "AB");.

As far as wide characters go, code with wide characters won't pass semantic analysis because there's no StrSplit overload that accepts wide characters.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 4:05 AM
lebedev.ri marked an inline comment as done.Jul 9 2019, 4:26 AM
lebedev.ri added inline comments.
clang-tools-extra/test/clang-tidy/abseil-faster-strsplit-delimiter.cpp
55

Cool, i have no further comments here then, thanks.