Page MenuHomePhabricator

[clang-format] Fix bug in parsing `operator<` with template
ClosedPublic

Authored by pjessesco on Jan 15 2022, 8:11 AM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/44601.

This patch handles a bug when parsing a below example code :

template <class> class S;

template <class T> bool operator<(S<T> const &x, S<T> const &y) {
  return x.i < y.i;
}

template <class T> class S {
  int i = 42;
  friend bool operator< <>(S const &, S const &);
};

int main() { return S<int>{} < S<int>{}; }

which parse < <> as << >, not < <> in terms of tokens as discussed in discord.

  1. Add a condition in tryMergeLessLess() considering operator keyword and >
  2. Force to leave a whitespace between tok::less and a template opener
  3. Add unit test

Diff Detail

Event Timeline

pjessesco requested review of this revision.Jan 15 2022, 8:11 AM
pjessesco created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2022, 8:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pjessesco updated this revision to Diff 400290.Jan 15 2022, 8:25 AM

Add deleted newline in FormatTest.cpp by mistake

pjessesco updated this revision to Diff 400341.Jan 15 2022, 9:11 PM

Fix to pass unittest

MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay added inline comments.Jan 17 2022, 9:43 AM
clang/lib/Format/FormatTokenLexer.cpp
432

isn't this going to crash if Tokens.size() is 3?

curdeius edited the summary of this revision. (Show Details)Jan 17 2022, 9:44 AM
curdeius added inline comments.Jan 17 2022, 9:54 AM
clang/lib/Format/FormatTokenLexer.cpp
432

It probably will. Anyway, it's a UB. Please do it differently. Also, please fix the typo: "Fourth".

clang/unittests/Format/FormatTest.cpp
9466

Please add a test for a possible crash: something like verifyFormat("< <>");

pjessesco updated this revision to Diff 400697.Jan 17 2022, 8:14 PM
pjessesco marked 2 inline comments as done.

Fix to accept feedbacks.

  1. Forth -> Fourth
  2. Add unit test verifyFormat("< <>");
  3. Fix to avoid undefined behavior
curdeius edited the summary of this revision. (Show Details)Jan 18 2022, 12:17 AM
curdeius accepted this revision.Jan 18 2022, 12:33 AM

LGTM.
Thanks for working on this!

Do you have commit rights or do you need that someone lands it for you?
If you need help, please indicate your name and email to be used for the commit.

Also, if you think contributing more patches, have a look at https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

clang/lib/Format/FormatTokenLexer.cpp
433–446

I hate the naming here.
Unless I'm mistaken we have: Fourth, First + 0, First + 1, First + 2.
But I'll clean it up at some time.

This revision is now accepted and ready to land.Jan 18 2022, 12:33 AM
MyDeveloperDay requested changes to this revision.Jan 18 2022, 12:33 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
9570

just make sure there isn't a whitespace change here you didn't mean

9657

you doing something in your editor...

This revision now requires changes to proceed.Jan 18 2022, 12:33 AM

run git clang-format before submitting (if making the patch from staged files)

owenpan added inline comments.Jan 18 2022, 1:23 AM
clang/lib/Format/FormatTokenLexer.cpp
433–446

Me too. Maybe changing First to point at Fourth and get rid of Fourth, which has a different level of indirection than First now. I would probably get rid of FourthTokenIsLess as well.

pjessesco updated this revision to Diff 400782.Jan 18 2022, 2:48 AM

Fix revision after running git clang-format.

Sorry for annoying, I thought I did not make any changes at that line. :(

name : Jino Park
email : pjessesco@gmail.com

MyDeveloperDay accepted this revision.Jan 19 2022, 11:48 PM
This revision is now accepted and ready to land.Jan 19 2022, 11:48 PM
curdeius added inline comments.Jan 19 2022, 11:52 PM
clang/unittests/Format/FormatTest.cpp
9466

Looking at other related bug reports, I think that we should test operator<< <>(); as well. Could you add it please?

Or I'll add it when landing.

This revision was landed with ongoing or failed builds.Jan 19 2022, 11:59 PM
This revision was automatically updated to reflect the committed changes.