This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers][NFC] integerLiteral(): Mention negative integers in documentation.
ClosedPublic

Authored by courbet on Jul 10 2017, 4:53 AM.

Details

Summary

Trying to match integerLiteral(-1) will silently fail, because clang only represents positive integer literals.

  • Update the documentation to explain how to match negative integers.
  • Add a unit test.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Jul 10 2017, 4:53 AM
aaron.ballman added inline comments.Jul 10 2017, 9:10 AM
include/clang/ASTMatchers/ASTMatchers.h
3824–3825 ↗(On Diff #105839)

I'm not keen on this explanation -- the reason you cannot match a negative integer is because the source languages treat the integer literal as positive and you apply the unary negation operator to the integer literal. Basically, from the lexer's perspective, the integer literal does not have a - sign; that is an expression.

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
671–674 ↗(On Diff #105839)

Was this formatting produced by clang-format?

courbet added inline comments.Jul 10 2017, 9:30 AM
include/clang/ASTMatchers/ASTMatchers.h
3824–3825 ↗(On Diff #105839)

What about "Note that you cannot directly match negative integers, as clang's AST represents a negative c/c++ integer literal as a UnaryOperator(IntegerLiteral())
" ?

aaron.ballman added inline comments.Jul 10 2017, 10:56 AM
include/clang/ASTMatchers/ASTMatchers.h
3824–3825 ↗(On Diff #105839)

That still implies this is somehow a Clang AST issue to work around when it is in fact the AST representing the language semantics. How about:

"You cannot directly match a negative numeric literal because the minus sign is a unary operator whose operand is the positive numeric literal. Instead, you must use a unaryOperator() matcher to match the minus sign:
<code example>"

This also handles the fact that this behavior isn't specific to integer literals (it also applies to float literals as well).

courbet updated this revision to Diff 105971.Jul 11 2017, 1:20 AM
courbet marked 4 inline comments as done.
  • Update doc
  • clang-format
include/clang/ASTMatchers/ASTMatchers.h
3824–3825 ↗(On Diff #105839)

Ah right, I was convinced that the "-" was part of the literal in c++ and that it was a clang AST oddity. I've re-read http://en.cppreference.com/w/cpp/language/integer_literal and realized my mistake.

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
671–674 ↗(On Diff #105839)

No, I disabled it because it adds a lot of noise (the whole file has lots of changes). Applied only to these lines.

aaron.ballman accepted this revision.Jul 11 2017, 5:39 AM

LGTM, thank you!

unittests/ASTMatchers/ASTMatchersNodeTest.cpp
671–674 ↗(On Diff #105839)

In case you were unaware, you can apply clang-format to a patch, too (it only affects the lines in the diff): https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

This revision is now accepted and ready to land.Jul 11 2017, 5:39 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the tip !