Page MenuHomePhabricator

[clang-tidy] Fixed checker for abseil to work in C++17 mode
AbandonedPublic

Authored by jvikstrom on Jun 11 2019, 2:42 AM.

Details

Reviewers
hokein
gribozavr
Summary

Fixes the checker for abseil to make tests pass in C++17 mode

Event Timeline

jvikstrom created this revision.Jun 11 2019, 2:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 2:42 AM
Herald added a subscriber: xazax.hun. · View Herald Transcript

Thanks for investigating these, nice work!

Some initial comments:

  • since your patch fixes four different check, I'd suggest to separate it (one patch per check)
  • there are some non-functional changes (code format), I'd avoid them in this patch (we could address them in a separate patch)
  • it would be nice if you could briefly describe the AST difference between C++11 and C++17 (how the fix work) in the patch description or comments
clang-tools-extra/test/clang-tidy/abseil-duration-unnecessary-conversion.cpp
1

I believe the tests are also passed for C++2a? use C++11-or-later will do the trick.

gribozavr added inline comments.Jun 11 2019, 5:01 AM
clang-tools-extra/clang-tidy/abseil/FasterStrsplitDelimiterCheck.cpp
86

hasDescendant will find any child, no matter how deeply nested. I think in this case we only want the expression itself to be equal to ByAnyChar call. Shouldn't this code use the elidable matcher you're adding in another file?

121

Unrelated edit?

clang-tools-extra/clang-tidy/abseil/TimeSubtractionCheck.cpp
34

Ditto, hasAncestor can go too far up the tree.

jvikstrom abandoned this revision.Jun 13 2019, 6:38 AM

Resubmitted as 4 different CLs