[PATCH] D136036: [Clang] Add __has_constexpr_builtin support

Mon Oct 17 14:08:26 PDT 2022

Izaron added a comment.

In D136036#3862065 <https://reviews.llvm.org/D136036#3862065>, @aaron.ballman wrote:

> Can you also add documentation and a release note for the new feature testing macro?

In D136036#3862649 <https://reviews.llvm.org/D136036#3862649>, @shafik wrote:

> Please add some of this context to the release notes and/or documentation, thank you.

Hi! Added some text to the docs and to the release notes mentioning `<cmath>`. Please look at my wording =)

In D136036#3862221 <https://reviews.llvm.org/D136036#3862221>, @yaxunl wrote:

> need some Sema tests to verify a constexpr builtin is const evaluated.

Hi! If I understood you correctly, every (or over 95%?) such function already has its tests checking its constant evaluation. For example test/Sema/constant-builtins-fmax.cpp <https://github.com/llvm/llvm-project/blob/main/clang/test/Sema/constant-builtins-fmax.cpp> checks constant `__builtin_fmax`.

In D136036#3862685 <https://reviews.llvm.org/D136036#3862685>, @shafik wrote:

> The changes to the various `Vist*CallExpr` functions don't seem directly related to supporting `__has_constexpr_builtin`, can you explain the purpose of those changes? If they are not directly related maybe it makes more sense for them to be split off into a follow-up PR?

We have a protocol that the `E` character in attributes marks that the builtin can be constant evaluated.

But the actual constant evaluation is done in `lib/AST/ExprConstant.cpp` and has little to do with builtin attributes.

So how do we keep the consistency of this protocol? In other words, how can we guarantee that IF the builtin is constant evaluated, it DOES have the `E` in `Builtins.def`?

I solved it with changes to `Visit*CallExpr`. Whenever we support constant evaluation in a builtin, we won't forget to mark its attributes with `E` (otherwise the evaluator returns early).

I was thinking to make a consistency test (outside the Clang code), but couldn't do it =(

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136036/new/

https://reviews.llvm.org/D136036