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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
