This is an archive of the discontinued LLVM Phabricator instance.

Add `BoolLiteralExpression` to SyntaxTree
ClosedPublic

Authored by eduucaldas on Jun 22 2020, 9:06 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Jun 22 2020, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2020, 9:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas marked 3 inline comments as done.Jun 22 2020, 9:14 AM
eduucaldas added inline comments.
clang/include/clang/Tooling/Syntax/Nodes.h
268

Homogenize this comments for other literals, and possibly other expressions. To be done in future change

269

Unify Literals under a Literal class, following the grammar, [lex.literal]
To be done in a future change

clang/unittests/Tooling/Syntax/TreeTest.cpp
54

We could use this to extend the coverage of our tests from isCXX to !isC.
To be done in a future change

gribozavr2 added inline comments.Jun 22 2020, 9:31 AM
clang/unittests/Tooling/Syntax/TreeTest.cpp
1243

C99 has bool literals, but the program should include stdbool.h.

I feel like it is better to make the predicate something like "hasBoolType()" and change the test to include stdbool.h.

eduucaldas marked an inline comment as done.Jun 23 2020, 12:36 AM
eduucaldas added inline comments.
clang/unittests/Tooling/Syntax/TreeTest.cpp
1243

stdbool consists on macro definitions, mapping booleans to integers. true is preprocessed into 1 and false to 0 .
I don't think there is a reasonable way of getting the proper SyntaxTree from that macro expansion

Additional problem, we don't have the test infrastructure for includes, AFAIK ^^

Finally, regarding the predicates, I prefer if they relate to languages, otherwise we might create many predicates that test for exactly the same thing, e.g. we would have hasBoolType() and hasNullPtr() that ultimately do the same thing, test if Language is not C

gribozavr2 accepted this revision.Jun 24 2020, 2:49 AM
gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.
clang/unittests/Tooling/Syntax/TreeTest.cpp
1243

true is preprocessed into 1 and false to 0 .

I see -- yes, that would make a completely different syntax tree.

Finally, regarding the predicates, I prefer if they relate to languages, otherwise we might create many predicates that test for exactly the same thing

Generally, testing for features instead of product versions is considered better, as that leads to more future-proof code (for example, we learned this lesson again in the area of web extensions and standards). In future, Clang can start supporting a language feature in other language modes as an extension (for example, supporting _Atomic in C++ is already a thing), or the language standard itself can decide to incorporate the feature (for example, C 2035 could adopt the nullptr keyword). It is highly unlikely for complex features like templates, but may reasonably happen for simpler features.

This revision is now accepted and ready to land.Jun 24 2020, 2:49 AM
eduucaldas marked 2 inline comments as done.

isC -> hasBoolType

clang/unittests/Tooling/Syntax/TreeTest.cpp
1243

Generally, testing for features instead of product versions is considered better, as that leads to more future-proof code

I totally agree with all you said. My worry was more regarding the fact that the implementation of hasBoolType would merely check for the language anyways. So if, following what you said, true and false really became a feature of C35 then the code would still be broken.

But then I guess it would be easier to fix hasBoolType than finding all the places where we used just isC because booleans are not in C.

Specially if all these predicates were put in clang/Testing/CommandLineArgs.h or something similar where more people would be seeing it.

Which brings the question shouldn't we move these predicates up?

gribozavr2 marked an inline comment as done.Jun 25 2020, 5:26 AM
gribozavr2 added inline comments.
clang/unittests/Tooling/Syntax/TreeTest.cpp
1243

Sorry for miscommunication -- in this specific we could go either way, since the C bool feature is already implemented to have a very different AST than in C++, and given C's stability principles, it is unlikely to change.

I am trying to move this testing infra to a common place in https://reviews.llvm.org/D82179.

This revision was automatically updated to reflect the committed changes.