[PATCH] D82310: Add `BoolLiteralExpression` to SyntaxTree
Dmitri Gribenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 25 05:47:25 PDT 2020
gribozavr2 marked an inline comment as done.
gribozavr2 added inline comments.
================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:1240
+ true;
+ false;
+}
----------------
eduucaldas wrote:
> gribozavr2 wrote:
> > eduucaldas wrote:
> > > gribozavr2 wrote:
> > > > 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.
> > > [[ https://clang.llvm.org/doxygen/stdbool_8h_source.html | 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
> > > 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.
> > 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?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82310/new/
https://reviews.llvm.org/D82310
More information about the cfe-commits
mailing list